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 "Edit in Github" link #3427

Merged
merged 9 commits into from
Dec 21, 2017
Merged

Fix "Edit in Github" link #3427

merged 9 commits into from
Dec 21, 2017

Conversation

humitos
Copy link
Member

@humitos humitos commented Dec 20, 2017

The most important commit is eb264ee

Test here: b982bf7

and then linting... (with some fixings for pre-commit)

Closes #3203

Since the `type` wasn't in the response when syncing version and
retrieving this data the `APIVersion.type` was returning `UNKNOWN`
and then the `APIVersion.commit_name` was incorrect.

By adding the `type` to the API response the `Version` it's completed.

#3203
@@ -1,3 +1,3 @@
[flake8]
ignore = E125,D100,D101,D102,D103,D105,D106,D107,D200,D202,D211,P101,FI15,FI16,FI12,FI11,FI17,FI50,FI53,FI54,MQ101,T000
ignore = E125,D100,D101,D102,D103,D105,D106,D107,D200,D202,D211,D403,P101,FI15,FI16,FI12,FI11,FI17,FI50,FI53,FI54,MQ101,T000
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of constantly changing these.. any reason this is here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm adding it here because I also add it to prospector.

flake8 is used in pre-commit command and also in the IDE linter (emcas, vim, atom, pycharm, etc) if you have it configured.

We need a consistency between prospector and flake8.

@@ -40,6 +40,7 @@ pep257:
- D106 # Missing docstring in public nested class

# pydocstyle
- D403 # First word of the first line should be properly capitalized ('Github', not 'GitHub')
Copy link
Member

@ericholscher ericholscher Dec 20, 2017

Choose a reason for hiding this comment

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

GitHub is actually their branding, so I don't understand this comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

This comment is the error message that prospector produces. I'm pasting the comment here to understand why I'm ignoring this particular code.

In this case, D403 doesn't allow a docstring starting with GitHub string probably because it's CamelCase.

@humitos
Copy link
Member Author

humitos commented Dec 20, 2017

are we really testing for all this data? Seems like a brittle way to test this, and will fail on any change to the serializer.

My idea with this test is to avoid these kind of problems (adding / removing a field and do not notice it). So, I personally prefer to be notified by a test about these changes. Maybe there is a better way to do it but I don't know.

@ericholscher
Copy link
Member

@humitos that's fine. Should make a comment about that in the test, so we know in the future that's what it is testing for.

@ericholscher ericholscher merged commit b5660a6 into master Dec 21, 2017
@stsewd stsewd deleted the humitos/gh/edit-link branch August 15, 2018 22:15
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.

links to github are broken
2 participants