add python script to generate config based on gitlab group #337
Conversation
aschleifer
force-pushed
the
334_generate_gitlab_script_python
branch
from
6bcdc3c
to
0c45519
Compare
for more information, see https://pre-commit.ci
|
Ok, I saw that github automatically ran flake8 against the changes and I fixed the issues it showed. |
|
I must say I'm not a fan of all changes the |
Codecov Report
@@ Coverage Diff @@
## master #337 +/- ##
=======================================
Coverage 82.48% 82.48%
=======================================
Files 11 11
Lines 651 651
=======================================
Hits 537 537
Misses 114 114 Continue to review full report at Codecov.
|
Yeah, it formats to to what this is becoming more and more common in open source projects (not the precommit, but running I will look at this tonight / tomorrow you could also rebase it into your changes and squash as you'd like in the mean time |
I'm ok with running a formatter in a project to agree on the formatting. I'm just saying I myself are not too happy with one of the formatting options, but I can live with it.
Nah, I'm fine with leaving it as it is right now. Unless you want me to rebase this all into one perfect commit which brings in the script in one clean commit. |
Based on the commits in the PR, i will likely do "squash and merge" which will have the same effect when merged to the trunk |
|
@aschleifer Is this good to merge for now? Anything else you'd like me to add? |
From my side this is good to go. I guess in the end the only thing missing is the already mentioned example entry. |
|
@aschleifer This sounds good. Thank you for this! Merging this in This coming week I will make a release that ships a root command with |
| args = vars(parser.parse_args()) | ||
| gitlab_host = args["gitlab_host"] | ||
| gitlab_namespace = args["gitlab_namespace"] | ||
| config_filename = args["config_file_name"] |
There was a problem hiding this comment.
You might just want to create a pathlib.Path here, as it has easy object based handling (e.g. Path.is_file())
There was a problem hiding this comment.
@dvzrv @aschleifer would either of you like to make a follow up PR improving upon this?
There was a problem hiding this comment.
@tony I will do a follow up PR soon to address this point.
| try: | ||
| if os.path.isfile(config_filename): | ||
| result = input( | ||
| "The target config file (%s) already exists, \ |
There was a problem hiding this comment.
As this codebase is for python >= 3.7, you can just use f-strings PEP0498. E.g. f"foo {my_variable}"
There was a problem hiding this comment.
@tony how do you stand on the topic of injecting variables into strings? is there one formatting / style you would like to see used in the python code?
|
@aschleifer Added to https://vcspull.git-pull.com/config-generation.html#python-version Similar to #336, I included my demo usage there. |
|
@aschleifer P.S. Is it your desire to have the scripts included in the python package itself? Either inside the directory or available in the |
I think at the end I will try to add this as a "native command" in vcspull and then we can remove these scripts. |
@tony this is the python version of the script we added with #336 for #334 .
It has the same usages as for the bash script, but it also validates that it can write the file and will ask the user if it should overwrite an existing config.
As I said before I'm not a python expert, so if you have any improvement ideas please let me know.
Also I wasn't sure about any style guides this project follows. If you name me the guide I'm happy to format the code accordingly.
Best regards
Segaja