Skip to content

IshaanG/week1#3

Open
IshaanG wants to merge 14 commits intoietbitmesra:masterfrom
IshaanG:IshaanG/week1
Open

IshaanG/week1#3
IshaanG wants to merge 14 commits intoietbitmesra:masterfrom
IshaanG:IshaanG/week1

Conversation

@IshaanG
Copy link

@IshaanG IshaanG commented Dec 16, 2019

No description provided.

@@ -0,0 +1,45 @@
import requests
Copy link
Member

@karngyan karngyan Dec 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should consider adding Module/Class/Method and docstring as well.
It's easier for coders to understand what your module/class/method does.

Also how about sorting the import orders as well.
You can refer this: https://www.python.org/dev/peps/pep-0008/#imports

data_all.append(data)
with open('data.json', 'w', encoding='utf-8') as f:
json.dump(data_all, f, ensure_ascii=False, indent=4)

Copy link
Member

@karngyan karngyan Dec 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method doesn't manipulate class properties or states. It might be a good idea to use @staticmethod decorator.

What do you say?

@@ -0,0 +1,42 @@
import spec
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add documentation and correct the import order here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants