Refactor meta.yaml lint logic #1906
Conversation
+ minor changes and fixes
this will be addressed separately
there are still some TODOs and failing tests
Non-noarch spelling, Literal instead of Union for enum values
replaced by enum order
- test must be package for imports to work - import AbstractSet
ytausch
force-pushed
the
refactor-metayaml-lint
branch
from
b032dd7
to
a654438
Compare
|
Cc @0xbe7a |
| return cls(hints=[hint]) | ||
|
|
||
|
|
||
| Linter = Callable[[dict, T], LintsHints] |
There was a problem hiding this comment.
Since every lint is expected to return a LintsHints anyway, why not provide one as an argument and omit the return type. Requiring lint to create an empty LintHints container even if none are returned seems a bit verbose at times
There was a problem hiding this comment.
So you're proposing an intentional side effect? I tried to avoid that with my current solution, although it's more verbose, yes.
| "Use linters_meta_yaml.is_jinja_line instead.", | ||
| DeprecationWarning, | ||
| ) | ||
| return linters_meta_yaml.is_jinja_line(line) | ||
|
|
||
|
|
||
| def selector_lines(lines): |
There was a problem hiding this comment.
Are any of these util functions actually used outside of conda-smithy @beckermr? I couldn't find any uses in the conda-forge org outside of this repo. In general, I think we should design a clearly specified public API in the long-term instead of assuming that every public function in this module is a potentially public API. As far as I can tell, other components only use conda_smithy.lint_recipe.main.
There was a problem hiding this comment.
+1
There are good Python libraries I've seen that prefix private modules and functions with _ to create a clear distinction between public and private API.
|
Any public method should be assumed to be used. As I said before on another pr, unless we plan to release v4, there can be no api changes. |
|
Also, we should not be introducing any deprecation processes unless core is in agreement on how they work. |
Where should we discuss this? Probably a new issue in this repo? Still, I'm wondering who the users could be that depend on these functions? I would expect that they are only used inside of conda-forge and thus I would expect that the only thing to do would be to ensure that these functions are no longer used anywhere in our codebase. |
|
Yes a new issue is fine and then on a core call. We're going to bust something for somebody even if we don't know about it. We use semver and that has meaning. |
|
Also while I'm not opposed to making the functions shorter here, the style in which these PRs are being made is making them difficult/impossible for me to review.
|
Checklist
newsentryThis builds upon #1900 by refactoring the meta.yaml lints out of the ~700 lines
lintify_meta_yamlfunction, modularizing the lint logic. While doing that, I added early returns, simplified code and fixed smaller issues.I think a modularized logic like this is a lot more maintainable, but the path forward should definitely be using more Pydantic models for interacting with complex JSON / YAML files.
#1900 needs to be merged first.