-
Notifications
You must be signed in to change notification settings - Fork 1
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
RDRP- 997 Replace toml.load with tomli.load #376
Conversation
@@ -1,6 +1,7 @@ | |||
"""Define helper functions to be used throughout the pipeline..""" | |||
import yaml | |||
import toml | |||
import tomli |
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.
the toml package is no longer used after switching to tomli. "import toml" can therefore be removed
src/outputs/export_files.py
Outdated
@@ -49,7 +50,7 @@ def get_schema_headers(config: dict): | |||
|
|||
# Get the headers for each | |||
schema_headers_dict = { | |||
output_name: toml.load(path) for output_name, path in schema_paths.items() | |||
output_name: tomli.load(path) for output_name, path in schema_paths.items() |
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.
unlike in "toml" , in "tomli" method load can't take in a string: it takes a binary (the one you would get with open(..) as f.
In order to read a string try "loads" (with an s) instead.
src/utils/helpers.py
Outdated
@@ -44,7 +45,7 @@ def user_config_reader(configfile: str = user_config_path) -> dict: | |||
{'title': 'TOML Example config', 'period': {'start_period': | |||
datetime.date(1990, 10, 10), 'end_period': datetime.date(2000, 10, 5)}} | |||
""" | |||
toml_dict = toml.load(configfile) | |||
toml_dict = tomli.load(configfile) |
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.
unlike in "toml" , in "tomli" method load can't take in a string: it takes a binary (the one you would get with open(..) as f.
In order to read a string try "loads" (with an s) instead.
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.
Just one question to address. Cheers.
Replaced toml.load with tomli.load, as it is faster and is supported by Python 3.11.
Added toml decode error exception as a co-pilot suggestion, not 100% its effective as it is not returning anything at the end of the statement.
Code
Documentation
Any new code includes all the following forms of documentation:
Args
andreturns
for all major functionsData
Testing
Peer Review Section
requirements.txt
Final approval (post-review)
The author has responded to my review and made changes to my satisfaction.
Review comments
Insert detailed comments here!
These might include, but not exclusively:
that it is likely to interact with?)
works correctly?)
Your suggestions should be tailored to the code that you are reviewing.
Be critical and clear, but not mean. Ask questions and set actions.