-
Notifications
You must be signed in to change notification settings - Fork 0
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
2-metadata-importation-doc #3
Conversation
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.
We discussed in person to be more specific with actors.
Probably want to identify more clearly BEFORE processes and AFTER processes as the sequence diagrams you're presenting seems to be applicable to current situation.
instead of putting yaml embedded in the doc, create an example structure and a pydantic validator (with auto-generated doc)
We discussed in person. The documentation needs to be reworked to offer a better understanding of the upcoming process within Nachet. |
should also add the github workflow checks as I see a couple basic issues that are detected by the checks: |
I showed my doc to @JolanThomassin and took his feedback. I've then added small description to the graphs. |
Very good graphs 👍 |
@FrancoisWerbrouck-CFIA I see a thumbs up but what we need is a reference to a new issue describing the work to be done to ensure the work is properly scheduled. |
file-validator.py
Outdated
|
||
def is_yaml_file(file_path): | ||
_, file_extension = os.path.splitext(file_path) | ||
return file_extension.lower() == '.yaml' |
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.
yml is also a very common extension
more reliable to look at headers than extension to determine file types:
file-validator.py
Outdated
# Ask Data (I dont have access to the data url atm) | ||
clientEmail=input("clientEmail: ") | ||
clientExpertise= input("Expertise: ") | ||
clientData=ClientData(clientEmail=clientEmail,clientExpertise=clientExpertise) |
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.
you're inconsistent in how you write your code. here the whitespaces before adn after the = signs (the rule is spaces except for parameters to fucntions). ruff extension should be able to give you feedback here.
file-validator.py
Outdated
yaml_data = yaml.safe_load(file) | ||
try: # Validate and parse YAML data using Pydantic model | ||
data = Index(**yaml_data) | ||
except Exception as error: # Errtor raised by Pydantic Validator |
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.
Catch the specific exception, not every type of exception. do not want to hide other type of exceptions.
file-validator.py
Outdated
try: # Validate and parse YAML data using Pydantic model | ||
data = Index(**yaml_data) | ||
except Exception as error: # Errtor raised by Pydantic Validator | ||
print(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.
should the program really continues without failing? typically should re-raise your custom exception to ensure the caller handles failure here.
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.
Why have two different PR open?
The basics of all the documentation on the current metadata and the database is completed. There also is in place a basic structure to perform queries on the database and the setup of said database. This encapsulate the core of the issue #2 |
cc5b5ba
to
d221bab
Compare
@Francois-Werbrouck my previous opened conversations are not acknowledged or commented? |
@rngadam The changes have already been made in other versions and then improved upon. (Index -> Session -> picture_set) By having old PRs waiting for review with already alot of increment and changes upon the structure, I've applied the latest changes to the latest version now being: #13 |
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'll follow up in your newer PR
Add: Documentation about the metadata importation