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

Fix Numerous Spelling, Style, English Issues #2139

Merged
merged 1 commit into from
Aug 22, 2016
Merged

Conversation

waprin
Copy link
Contributor

@waprin waprin commented Aug 18, 2016

A 30 min quick, but slow and deliberate, review of the existing codebase.

  • Multiple typos and misspellings
  • Multiple missing or incorrect punctuation
  • Unused variables

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

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 18, 2016
@waprin
Copy link
Contributor Author

waprin commented Aug 18, 2016

@dhermes @tseaver

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

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 % vs. format. I used to be all format all the time but found it's hard to adhere to in practice (e.g. logging.log() assumes % formatting)

@theacodes
Copy link
Contributor

hard to adhere to in practice (e.g. logging.log() assumes % formatting)

I just ignore logging's sugar. .format is definitely preferable.

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.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

- 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.
@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

OK finished my pass and don't see many problems. I'd really like to hear the why on adding all the . but the typos are obvious so thanks.

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

@waprin I see you made a new commit since I originally reviewed. Care to point me to what (in the 28 files) has changed?

@waprin
Copy link
Contributor Author

waprin commented Aug 18, 2016

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.

@dhermes
Copy link
Contributor

dhermes commented Aug 18, 2016

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.

@waprin
Copy link
Contributor Author

waprin commented Aug 18, 2016

Yes I agree 1000% that the more we can do with tools the better.

@dhermes dhermes merged commit 467ed7d into googleapis:master Aug 22, 2016
@dhermes dhermes mentioned this pull request Sep 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants