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

Add missing parameters, do not pass None #144

Merged
merged 5 commits into from
Jan 16, 2023
Merged

Conversation

Kamilcuk
Copy link
Contributor

@Kamilcuk Kamilcuk commented Jan 3, 2023

Add missing parameters to allocations.get_allocations and jobs.get_jobs.
Do not pass prefix if it's none (or any other param). This simplifies the code handling with optional parameters. If prefix is passed "", then allocations are sorted alphabetically, otherwise chronologically.

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2023

Codecov Report

Merging #144 (155225c) into master (78452b6) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
- Coverage   90.67%   90.58%   -0.10%     
==========================================
  Files          31       31              
  Lines        1287     1285       -2     
==========================================
- Hits         1167     1164       -3     
- Misses        120      121       +1     
Impacted Files Coverage Δ
nomad/api/allocations.py 100.00% <100.00%> (ø)
nomad/api/base.py 94.31% <100.00%> (-1.14%) ⬇️
nomad/api/jobs.py 89.79% <100.00%> (-0.21%) ⬇️

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

Copy link
Collaborator

@nikita-b nikita-b left a comment

Choose a reason for hiding this comment

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

@Kamilcuk Thanks for your contribution! I only left a small comment.

self,
prefix: Optional[str] = None,
namespace: Optional[str] = None,
filter: Optional[str] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

My suggestion is: Let's use _filter here instead of builtin value.

I think it's better to be slightly different from the official API than shadow keywords.

@Kamilcuk
Copy link
Contributor Author

I am under the influence of sqlalchemy which uses in_ with a trailing _. Leading _ suggests an internal symbol.

@nikita-b
Copy link
Collaborator

@Kamilcuk Yep, it seems you are right:
https://peps.python.org/pep-0008/

_single_leading_underscore: weak “internal use” indicator. E.g. from M import * does not import objects whose names start with an underscore.
single_trailing_underscore_: used by convention to avoid conflicts with Python keyword, e.g.

LGTM

@nikita-b nikita-b self-requested a review January 16, 2023 15:11
@nikita-b nikita-b merged commit fb8afc2 into jrxFive:master Jan 16, 2023
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