Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ran all python code through black, code formater #16

Closed
wants to merge 1 commit into from
Closed

Ran all python code through black, code formater #16

wants to merge 1 commit into from

Conversation

de-sh
Copy link
Contributor

@de-sh de-sh commented May 19, 2019

Fixes un-formatted code with black

Checklist

  • My branch is up-to-date with upstream/develop branch.
  • Everything works and tested for Python 3.5.2 and above.

Description

The Black code formatter library is a python standard for maintaining readable code bases, I have run the code of the parser through it to make it more readable and PEP8 compliant.

Change logs

  • Code formatted with black

@@ -103,7 +107,7 @@ def valid_endpoint(path: str) -> str:
:return:
"""
# "collection" or true means valid
path_ = path.split('/')
path_ = path.split("/")
Copy link
Contributor

@Mec-iS Mec-iS Jun 3, 2019

Choose a reason for hiding this comment

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

you should be sure that the rules for the formatter stick to the conventions we established for the codebase. For example: we decide to always use ' single quotes for strings, except that for dictionaries meant to be dumped into JSON strings and for variables added via f-strings like f'Some text with a {dict["variable"]}'

In general you have to stick to how is done in hydrus.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -103,7 +107,7 @@ def valid_endpoint(path: str) -> str:
:return:
"""
# "collection" or true means valid
path_ = path.split('/')
path_ = path.split("/")
Copy link
Contributor

Choose a reason for hiding this comment

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

},
{
"@type": "SupportedProperty",
"property": "",
"readonly": "true",
"required": "false",
"title": "category",
"writeonly": "true"
"writeonly": "true",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the point of adding all these commas.

Please stick to pep8 package as a reference libary

@Mec-iS
Copy link
Contributor

Mec-iS commented Jun 3, 2019

See my review. We cannot really change the styling library every time, we have always been using pep8 package as far as I remember

@de-sh
Copy link
Contributor Author

de-sh commented Jun 3, 2019

See my review. We cannot really change the styling library every time, we have always been using pep8 package as far as I remember.

Ok, These were a suggestion based on the output of black, put it up for review, seems like this is not necessary... comments?

The PR can be closed if it seems to be unnecessary 👍

@chrizandr
Copy link
Member

chrizandr commented Jun 3, 2019

I agree, we already have PEP8 based code formatting in all other repos. It would make more sense to stick to it than to change the format to something new. I don't potentially see any benefit in switching over to black.

EDIT: If you can give some insights as to why you think this is better, we might be open to more discussion about this. Otherwise I will close the PR.

@chrizandr chrizandr closed this Jun 3, 2019
@chrizandr chrizandr reopened this Jun 3, 2019
@de-sh
Copy link
Contributor Author

de-sh commented Jun 8, 2019

@chrizandr I thought python/black is provided by the PSF and hence using it would be better. There is no other reasoning and I have since come to think of not using a no compromise code formatter. I believe it's better to close this PR

@de-sh de-sh closed this Jun 8, 2019
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.

3 participants