-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix Numerous Spelling, Style, English Issues #2139
Conversation
Cool to accept typo / grammar fixes but I'm not sure when punctuation is warranted / if we want to have an opinion on that. Similar on |
I just ignore logging's sugar. This change LGTM. |
@@ -221,7 +221,8 @@ def create(self, client=None): | |||
self._set_properties(api_response) | |||
|
|||
def exists(self, client=None): | |||
"""API call: test for the existence of the change set via a GET request | |||
"""API call: test for the existence of the change set via a GET | |||
request. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
- Dozes of typos and misspellings - Dozens of missing incorrect punctuation - Unused variables The library is also wildly inconsistent on its usage of % vs the format function for string interpolation. I would like to fix that as well but not sure what the standard is, I recommend the format function.
OK finished my pass and don't see many problems. I'd really like to hear the why on adding all the |
@waprin I see you made a new commit since I originally reviewed. Care to point me to what (in the 28 files) has changed? |
My bad I forgot to push commits again. I fixed the line that I moved to the next lien by removing an extraneous space. Will remember to push commits. I don't know about the punctuation, depends on whether docstring consistency is important. Most of the repo uses periods at the end of each docstring param description. |
We're good to merge here / LGTM I'm just trying to point out that human language is hard and enforcing these sorts of rules without a tool of some kind seems to be a big stretch goal. |
Yes I agree 1000% that the more we can do with tools the better. |
A 30 min quick, but slow and deliberate, review of the existing codebase.
The library is also wildly inconsistent on its usage of % vs the format function for string interpolation. I suggest format, but that belongs in its own PR.
http://stackoverflow.com/questions/12382719/python-way-of-printing-with-format-or-percent-form