Conversation
lalten
force-pushed
the
quote-colon-strings
branch
4 times, most recently
from
fa5c3f4 to
d618467
Compare
lalten
force-pushed
the
quote-colon-strings
branch
from
d618467 to
0b60ee2
Compare
|
@lyz-code Please have a look. |
|
I just noticed this also "fixes" |
lyz-code
left a comment
lyz-code
left a comment
There was a problem hiding this comment.
Hi @lalten for your contribution, it looks great. However there are a pair of polishing comments, tell me what do you think about them.
Regarding the dependency broken CI, I agree with you that it's out of the scope of the PR. Sadly I don't have time to address this myself so I propose you two solutions:
- Run
make updateand it will probably fix the issue - Disable the check in the CI configuration
Thanks
| - item | ||
| - item | ||
| ``` | ||
| - Quote jobs.*.containers.volumes entries in GitHub Actions workflow yamls to not confuse GitHubs Yaml parser. |
There was a problem hiding this comment.
I wouldn't say this is a feature, but a fix for an unexpected behavior, isn't it? If you agree I'd remove this line
| def _quote_gha_container_volumes( | ||
| self, source_dict: CommentedMap | CommentedSeq | ||
| ) -> None: | ||
| """Quote jobs.*.container.volumes entries.""" |
There was a problem hiding this comment.
I find the phrasing of the docstring confusing. Wouldn't it be clearer something like prevent list strings to be converted to dictionaries?
I'd add an example of what this function fixes (the one in the issue is just fine)
I'd also change the name of the method, as it's not specific to gha_container_volumes, there may be other strings with colons that will benefit from this method
2 participants
Fixes #283
Checklist