refactor conda-forge.yml linting logic, hint about extra fields

ytausch commented 3 months ago

Checklist

[x] Added a news entry

Supersedes #1865 .

recipe-lint now hints about additional fields as compared with the Pydantic schema. While working on it, I cleaned up the linting logic. A new PR will follow for recipe lints.

Cc @0xbe7a

ytausch commented 3 months ago

Which changes do you mean? I think the Python API changes that include lintify_forge_yaml?

I can re-add them with the old type signature and raise a deprecation warning.

Is there a deprecation process for conda-smithy? How do I mark functions for deletion in v4?

beckermr commented 3 months ago

I mean any public function in the package. We cannot just delete them at will.

I am not personally in favor of adding a deprecation process to smithy. The old APIs are just fine. However, the discussion of API changes and smithy v4 is something the core group as whole has to agree on.

All this PR needs to do is add hints on extra fields.

If you want to propose changes or cleanups to other functions, please use a separate PR.

ytausch commented 3 months ago

I understand that public functions need to stay intact. This is why I want to re-add them with a deprecation notice.

In my opinion, a deprecation process is needed to keep a modern API. A lot of things have changed since this code was initially written, and a lot of things are historic. Refactoring is part of keeping the code in a maintainable state. Here is a list of the things of the public API I want to change, together with my rationale for it:

lint_recipe.str_type is a redundant alias for str, it was added for supporting Python 2 and 3 (#871) but we don't need it anymore
lintify is not an English word
returning a tuple of two lists (lints, hints) is error-prone because you can very easily switch them around (in fact, I did it myself while working on it and I also saw some confusion about it in conda-smithy itself) - the data class approach resolves this while adding some elegance for adding lints together

Besides from that:

_format_validation_msg is not part of the public API

Also, I think the new Python consensus is that pathlib is superior of the old os.path functions. As far as I see though, making this change only affected new code and test code, so no breaking API changes.

The reason why I included all this in this PR (and not a separate one) is that I don't want to make our technical debt worse by building more stuff upon it. All of my changes directly interfere with the code I am adding, so IMO there are no unrelated changes.

ytausch commented 3 months ago

This PR should now contain no breaking API changes anymore.

ytausch commented 3 months ago

Related to my "fixing things as you see them" idea: https://softwareengineering.stackexchange.com/q/244807

ytausch commented 3 months ago

@xhochy @beckermr Was there any discussion about how these (technically) API changes should be handled? I would like this PR to move forward and am happy to contribute to a discussion if it hasn't happened yet.

xref https://github.com/conda-forge/conda-smithy/pull/1906#issuecomment-2041549358

isuruf commented 3 months ago

Can we please use one PR to do one thing? The refactoring shouldn't have to get blocked on hint about extra fields.

ytausch commented 3 months ago

Can we please use one PR to do one thing? The refactoring shouldn't have to get blocked on hint about extra fields.

Adding to my comment above: My fear is that refactorings simply don't happen if the policy is as strict as you propose. However, since you seem to really want separate PRs, I will try to separate refactorings and functional changes more into separate PRs since I see a point in being able to review easily. Here, I pointed out the only functional change above.

xhochy commented 3 months ago

However, since you seem to really want separate PRs, I will try to separate refactorings and functional changes more into separate PRs since I see a point in being able to review easily.

Just to second this: For me, as someone who only "briefly" knows the codebase, this PR is quite hard to review to the mix of refactoring and functional changes. I would be able to give better feedback if only small things change. You can make larger PRs to illustrate the context, but we should aim to have small ones that are easily reviewable and thus can be merged quite quickly.

ytausch commented 3 months ago

@isuruf @beckermr Since our discussion so far was very general (which is fine) I would like to know what exactly needs to happen with these changes here so that you are able to review them. Is it sufficient to incorporate the things we discussed in new PRs or is some finer grained separation also necessary here?

jaimergp commented 2 months ago

Agreed with others that there are many changes (which don't get me wrong, they are welcome!) that get in the way of reviewing this PR with proper attention. I tuned out a couple times after a few os.path.join removals.

I'm in favor of having these QoL improvements done but they should be easy to review (e.g. one type of change per commit), and scoped in their own PR. These refactors are usually easy to review when the PR only changes that and doesn't change behavior.

My advice here is to split this PR into two PRs:

The minimal changeset that implements the extra field hints.
A general QoL changes PR, possibly supported by tooling (e.g. ruff, pyupgrade, etc). The idea here would be to agree on some config to apply automatically to the codebase and then regenerate the PR as necessary.

Let me know if you need assistance!

ytausch commented 2 months ago

The functional part of this PR moved to #1920. I will clean this up here, please move any discussion about these parts there.

conda-forge / conda-smithy

refactor conda-forge.yml linting logic, hint about extra fields #1900