-
Notifications
You must be signed in to change notification settings - Fork 46
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: Allow yml
extension
#169
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.
@tbrlpld this seems spot on to me – we have to attempt to read both paths either way, so I don’t think there would be much of a better way. I have a feeling this is overkill but we could cache whichever file extension was loaded successfully last for a given pattern to avoid the extra IO when a site consistently uses .yml
?
To finish the PR, I’d additionally want to see:
- A unit test for this
- A mention in the documentation, replacing the "Always use
.yaml
" we have here: https://torchbox.github.io/django-pattern-library/reference/api/#yaml-structure
@thibaudcolas I have pushed changes according to your review. Regarding the caching: I also think that would be overkill. Especially in larger projects I can imagine that both extensions could occur when no linter or CI checks preventing it from happening. In that case it would be particularly confusing why the files are not recognized while they would work in another project which is consistent. If we wanted to allow projects to be consistent, we could add a setting, but I am afraid that could also cause confusion because you would need to know how it project is configured. I am not sure, but would using |
@tbrlpld thank you, I’ll try to review this once I get the chance. Re |
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.
Thanks Tibor, this is great.
5d22386
to
5564445
Compare
Thanks for reviewing @bcdickinson |
5564445
to
d28cf28
Compare
Description
So far, the context YAML files had to have the
.yaml
extension. YAML files often also use the.yml
extension. This PR adds a second attempt at finding the YAML file with the.yml
extension after the.yaml
extension has failed to produce a result.The second attempt is simply enabled by adding a second nested try-except-block. I am not sure this is the most elegant solution, but it works and seems easy to understand.
Fixes #161
Checklist