search

jrxFive / python-nomad

Client library Hashicorp Nomad
https://python-nomad.readthedocs.io/en/latest/
MIT License
141 stars 74 forks source link

Add pylint #143

Closed nikita-b closed 1 year ago

nikita-b commented 1 year ago

I want to add pylint to the repo before other changes.

It's only WIP.

codecov-commenter commented 1 year ago

Codecov Report

Merging #143 (221d0b6) into master (490b148) will increase coverage by 0.02%. The diff coverage is 81.78%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   91.10%   91.13%   +0.02%     
==========================================
  Files          31       31              
  Lines        1271     1275       +4     
==========================================
+ Hits         1158     1162       +4     
  Misses        113      113
Impacted Files Coverage Δ
nomad/api/namespaces.py 57.50% <20.00%> (ø)
nomad/api/namespace.py 51.35% <35.29%> (ø)
nomad/api/scaling.py 76.19% <42.85%> (-3.81%) :arrow_down:
nomad/api/event.py 85.36% <60.00%> (+3.14%) :arrow_up:
nomad/api/client.py 87.91% <67.85%> (+0.13%) :arrow_up:
nomad/api/deployments.py 90.24% <80.00%> (ø)
nomad/api/node.py 87.69% <82.75%> (ø)
nomad/api/jobs.py 90.00% <85.71%> (ø)
nomad/api/nodes.py 88.63% <86.66%> (+0.26%) :arrow_up:
nomad/api/acl.py 91.42% <87.50%> (-0.24%) :arrow_down:
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

nikita-b commented 1 year ago

@jonathanrcross @jrxFive Hi, I added pylint to the pipeline and fixed the code. I added a lot of exceptions for backward compatibilities except renaming id to _id everywhere. Motivation:

  1. It's easy to make mistake with id.
  2. Most likely people don't use the named id argument.
  3. We have 2.0.0 release and technically it's a major release.

If you have a different opinion, I can remove these changes from MR.

BTW, could you fix the docs polishing? https://python-nomad.readthedocs.io/en/latest/ It should work automatically as I understand but I don't have access and can't see any error in the process.

jonathanrcross commented 1 year ago

Agreed it should have never been shadowed id; yea if we're gonna be doing a major release might as well clean up this code smell.

I'll look into in the doc publishing issue, thanks for letting me know!

nikita-b commented 1 year ago

@jonathanrcross Could you please review MR, please? :)