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

Remove inappropriate lint messages #90

Merged
merged 3 commits into from Nov 22, 2019
Merged

Remove inappropriate lint messages #90

merged 3 commits into from Nov 22, 2019

Conversation

CasperWA
Copy link
Member

Also, update mongo transformers, satisfying linter - no content or behaviour changes, other than introducing some raising, where appropriate.

@codecov
Copy link

codecov bot commented Nov 21, 2019

Codecov Report

Merging #90 into master will decrease coverage by 0.04%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage    85.1%   85.06%   -0.05%     
==========================================
  Files          38       38              
  Lines        2216     2209       -7     
==========================================
- Hits         1886     1879       -7     
  Misses        330      330
Impacted Files Coverage Δ
optimade/models/jsonapi.py 88.63% <ø> (ø) ⬆️
optimade/validator/__init__.py 13.04% <ø> (ø) ⬆️
optimade/models/structures.py 83.33% <0%> (ø) ⬆️
optimade/models/entries.py 100% <100%> (ø) ⬆️
optimade/filtertransformers/mongo.py 75.19% <55%> (-1.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 080703c...f92d914. Read the comment docs.

@ml-evs
Copy link
Member

ml-evs commented Nov 21, 2019

Do you think we should add flake8 (or similar) to the travis? It can be very annoying to have flake8 issues reported on the same footing as test failures, so maybe we could try the new github actions?

Copy link
Contributor

@fekad fekad left a comment

Choose a reason for hiding this comment

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

Thanks, @CasperWA I think it looks nice. Please feel free to ignore my comments anytime (they are mostly about coding style more than functionality). There is one thing: I think the TransformerError never will be raised so it is misleading. The lark-parser runs in its own try block and it will raise its own error even before calling these functions. The only scenario when it could happen if you modify the grammar but in that case, you must update the Transformer as well...

optimade/filtertransformers/mongo.py Outdated Show resolved Hide resolved
optimade/filtertransformers/mongo.py Outdated Show resolved Hide resolved
optimade/filtertransformers/mongo.py Show resolved Hide resolved
optimade/filtertransformers/mongo.py Show resolved Hide resolved
optimade/filtertransformers/mongo.py Outdated Show resolved Hide resolved
@CasperWA
Copy link
Member Author

Do you think we should add flake8 (or similar) to the travis? It can be very annoying to have flake8 issues reported on the same footing as test failures, so maybe we could try the new github actions?

I was indeed considering adding the pre-commit run as a test, don't know if this will be enough?

@CasperWA
Copy link
Member Author

(...) There is one thing: I think the TransformerError never will be raised so it is misleading. The lark-parser runs in its own try block and it will raise its own error even before calling these functions. The only scenario when it could happen if you modify the grammar but in that case, you must update the Transformer as well...

I didn't know this, so I have removed TransformerError and adapted the class.

In order to combine both programming styles (using elif, while also sticking to the linting pointers) I have create local variables, where needed or been "clever" (see known_op_rhs). Hopefully the "cleverness" doesn't compromise the readability?

Concerning the non-existing elifs in set_op_rhs, they can be implemented alongside the actual implementation :)

@CasperWA
Copy link
Member Author

Do you think we should add flake8 (or similar) to the travis? It can be very annoying to have flake8 issues reported on the same footing as test failures, so maybe we could try the new github actions?

👍 for GitHub Actions. We are currently starting to use it in our other repos at EPFL, it seems really nice. Unfortunately, I have not been part of the people setting it up. If you have experience with GitHub Actions, I would push hard for this :)

@CasperWA CasperWA requested a review from fekad November 22, 2019 09:06
@ml-evs
Copy link
Member

ml-evs commented Nov 22, 2019

Do you think we should add flake8 (or similar) to the travis? It can be very annoying to have flake8 issues reported on the same footing as test failures, so maybe we could try the new github actions?

👍 for GitHub Actions. We are currently starting to use it in our other repos at EPFL, it seems really nice. Unfortunately, I have not been part of the people setting it up. If you have experience with GitHub Actions, I would push hard for this :)

I'll have a play around with this later today if I get time, does it make sense to add it in this PR?

@CasperWA
Copy link
Member Author

I'll have a play around with this later today if I get time, does it make sense to add it in this PR?

I wouldn't say it is. But we can make it, if you want. Then the title should also be changed though.
I think the cleanest is simply to just finish this and do GitHub Actions / extra checking and testing in a separate PR?

@ml-evs
Copy link
Member

ml-evs commented Nov 22, 2019

I wouldn't say it is. But we can make it, if you want. Then the title should also be changed though.
I think the cleanest is simply to just finish this and do GitHub Actions / extra checking and testing in a separate PR?

Roger that. I have no strong feelings about the style in this PR provided it now pleases the linter, so will accept and leave it for you to decide whether to wait for @fekad!

@CasperWA
Copy link
Member Author

Roger that. I have no strong feelings about the style in this PR provided it now pleases the linter, so will accept and leave it for you to decide whether to wait for @fekad!

Cheers. I will wait for @fekad, since I am touching the Mongo filtertransformer.

Copy link
Contributor

@fekad fekad left a comment

Choose a reason for hiding this comment

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

Looks perfect for me

@CasperWA CasperWA merged commit 3d3f906 into master Nov 22, 2019
@CasperWA CasperWA deleted the minor_lint_fixes branch November 22, 2019 14:02
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