-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature/setup param #49
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #49 +/- ##
===========================================
- Coverage 82.76% 69.19% -13.56%
===========================================
Files 5 6 +1
Lines 87 185 +98
Branches 10 38 +28
===========================================
+ Hits 72 128 +56
- Misses 6 42 +36
- Partials 9 15 +6 ☔ View full report in Codecov by Sentry. |
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 left some comments. Thank you for your efforts
tests/test_reserver.py
Outdated
# [get_random_name(), get_random_name() + get_random_name()], | ||
# ["config.json", "config2.json"] | ||
# ) == 2 | ||
assert True == True |
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.
Add an extra new line at the end of this file.
{ | ||
"description": "[config] This name has been reserved using Reserver", | ||
"author": "[config] Development Team", | ||
"author_email": "[email protected]", | ||
"url": "https://configurl.com", | ||
"download_url": "https://configdownload_url.com", | ||
"source": "https://configgithub.com/source", | ||
"license": "[config] MIT" | ||
} |
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.
Also, add an example of this to README.md
.
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.
after this PR, I will handle CLI in which we can handle custom batch uploads
then I will update README.md
reserver/util.py
Outdated
config_file = open(file_name) | ||
return json.load(config_file) | ||
else: | ||
raise ReserverBaseError(PARAM_FILE_DOES_NOT_EXIST_ERROR) |
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.
Add extra new line to the end of this file.
reserver/util.py
Outdated
if ".json" not in file_name: | ||
file_name = file_name + ".json" |
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.
That's not a good check. They may have a json file without .json
extension. This is a bit over-engineering.
reserver/reserver_param.py
Outdated
INVALID_PACKAGE_PARAMETER_VALUE_ERROR = "Invalid value for {parameter} that should be a valid {regex}" | ||
INVALID_CONFIG_FILE_NAME_ERROR = "Given file name for user-defined setup.py params is not a string." |
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.
Are you sure about setup.py
?
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 update the error message, changing setup.py
with package
.
reserver/reserver_param.py
Outdated
PARAM_FILE_DOES_NOT_EXIST_ERROR = "Given file doesn't exist." | ||
INVALID_INPUT_USER_PARAM = "Invalid input for user params." | ||
UNEQUAL_PARAM_NAME_LENGTH_ERROR = "You should pass either one single file path to be used for setup.py parameters \ |
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.
Not sure about setup.py
again.
Also, I prefer this:
`user_params_path` should be either a path or a list of paths to the config file(s).
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 update the error message, changing setup.py
with package
.
reserver/reserver_param.py
Outdated
INVALID_INPUT_USER_PARAM = "Invalid input for user params." | ||
UNEQUAL_PARAM_NAME_LENGTH_ERROR = "You should pass either one single file path to be used for setup.py parameters \ | ||
or per each package name, there should be a specific dedicated file path." |
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.
Also, add a new line at the end of this file.
b716523
to
4496359
Compare
Reference Issues/PRs
#12
#19
What does this implement/fix? Explain your changes.
Any other comments?