-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Added support of pyproject.toml file #41
Conversation
@DolajoCZ Thanks for the contribution! Overall it looks very good, but I would make a change: the config values from
It would be great to have a test case with config values set in different places. |
@rubik. Thank you for response. I will change priorities as you suggesting. Regarding last line |
@DolajoCZ Yes, exactly. A test case with values set from different places (even the same values) to verify the priority. |
@rubik Ok I can do it but I need to specify required behaviour in case when I have parameter |
@rubik I made changes as you required. Please let me know, if it is according your expectations. Thanks |
xenon/__init__.py
Outdated
continue | ||
|
||
# Set only in pyproject.toml | ||
setattr(args, arg_name, arg_value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of cloning the args object and checking if a name is present, it's better to simply:
- set all the values from pyproject
- set all the values from the command line
That way no cloning is needed, no checking is needed and the priority of the sources is taken care of automatically, since CLI options will override those from pyproject.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I understand it correctly. In second step when I need to get values set from cmd I also get arguments which were not passed by user but they have set default value.
Maybe if you can share piece of code to prove me where is my understanding incorrect. Then I will make necessary changes. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DolajoCZ We can:
- start with the default values
- override them with pyproject values (if any)
- override them with command line values (if any)
Then the priority is respected naturally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your proposed approach, but i dont understand how you want to get only arguments and values which were passed by user in cmd. If I use args = parser.parse_args()
I get arguments and values including arguments which were not set by user. Maybe I don't know appropriate command to get from parser
only realy passed values. Can you give me advice please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DolajoCZ You can remove the defaults from the ArgumentParser object and put them in a dictionary. Then if a user does not supply an option it will be None in the result of parser.parse_args()
. So you just skip the ones with value None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rubik thank you for advice. This was something which does not come to my mind. I made necessary modification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rubik is current code ok according your expectations or should I modify it? One thing whitch I found when I looked at other python packages witch support pyproject.toml
is that in case when parameter can be list of values (exclude for example), then even if you want use single value you shoud use ["value"]
instead of "value"
. Current implementation allow use "value"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rubik any comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any progress on this?
@DolajoCZ Looking great! I added some comments. |
@DolajoCZ Apologies for the delay, thank you so much for your contribution. I was unable to dedicate time to my open-source projects. |
I've added support of pyproject.toml file for ability to configure xenon for CI/CD from this one configuration file common for other tools.