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

Added fix for issue #107 #113

Closed
wants to merge 2 commits into from

Conversation

Aayush-Kasurde
Copy link
Contributor

Converted Google project name to lower case using lower()

@pypingou
Copy link
Member

Hi,

Thanks for the fix, couple of remarks:

a/ This will not fix the use case of #107 because now it will just take the project.name.lower() so it's as if the user had entered in anytia the project name in lower case.
It only needs fixing in the URL not for the regex.

b/ The commit message is good, but you just inverted.
The first line should be : Converted Google project name to lower case using lower() and lower down you would have Fixes issue #107
Otherwise the first thing you see when reading the log of the project is Added fix for issue_107 which is not really descriptive of what the change is (unless you know by heart every issue of the project).

@Aayush-Kasurde
Copy link
Contributor Author

a/ Could you please elaborate more ?

b/ Totally Agree. I will take care of this in future.

@pypingou
Copy link
Member

See in #107 (the first post, the original issue) what is a good and a bad URL and see how the sources are named ;-)

@cicku
Copy link

cicku commented Mar 18, 2015

It's better to squash into one commit.

@pypingou
Copy link
Member

And it would allow to adjust the commit message :)

Also, you could change only one line, no real need to keep the projectname variable :)

@Aayush-Kasurde Aayush-Kasurde deleted the issue_107 branch March 18, 2015 14:01
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.

None yet

3 participants