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

humitos added 6 commits Dec 20, 2017
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
ericholscher
Copy link
Member

ericholscher commented on b982bf7 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.

@@ -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

@ericholscher ericholscher Dec 20, 2017

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

Copy link
Member Author

@humitos humitos Dec 20, 2017

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

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

Copy link
Member Author

@humitos humitos Dec 20, 2017

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 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

@ericholscher ericholscher commented Dec 20, 2017

@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
1 check passed
@stsewd stsewd deleted the humitos/gh/edit-link branch Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants