conda-forge / conda-smithy

The tool for managing conda-forge feedstocks.
https://conda-forge.org/
BSD 3-Clause "New" or "Revised" License
151 stars 176 forks source link

feat: add rattler build linting #1979

Closed nichmor closed 2 months ago

nichmor commented 3 months ago

Checklist

nichmor commented 2 months ago

Hey! @beckermr and @isuruf ! I've added back linting of recipe.yaml. Could you please take a look? Thanks!

jaimergp commented 2 months ago

My two cents: now that we are going to have two implementations of many similar checks, is it time to add identifiers to each lint/hint rule in the same way that e.g. ruff or shellcheck do it? I imagine (ending up) having documentation for each rule explaining the rationale and workarounds if needed for example.

beckermr commented 2 months ago

To be clear we should not have two implementations. We need to reuse as much code as possible.

nichmor commented 2 months ago

My two cents: now that we are going to have two implementations of many similar checks, is it time to add identifiers to each lint/hint rule in the same way that e.g. ruff or shellcheck do it? I imagine (ending up) having documentation for each rule explaining the rationale and workarounds if needed for example.

hey @jaimergp ! I think it is a very good point!

I think we could have at least two explicit categories - hints ( H ) and lints ( L ) and then something specific for rattler-build and conda-build lint if there are any ( similar to how there are pylint or flake lints )

nichmor commented 2 months ago

I've used old API name so it will not create any breaking changes. Could you please @beckermr review it again?

nichmor commented 2 months ago

@beckermr I can strip tests out of this PR, so it would be just ~500 changes. and following PR will add them back so you can potentially focus only on tests.

wdyt?

beckermr commented 2 months ago

I'd go the other direction. Add a subset of the lints in each PR with the tests for those lints. Then we can look at the code+tests together.

nichmor commented 2 months ago

I understand the rationale behind reviewing code and tests together in smaller chunks. However, splitting this into multiple PRs with subsets of lints presents some challenges in my opinion:

Maybe we could do some iterations of reviewing this PR? so all the discussions will be here and it would be easier for me to keep track of them and apply.

nichmor commented 2 months ago

I'd go the other direction. Add a subset of the lints in each PR with the tests for those lints. Then we can look at the code+tests together.

after some additional thought, I also have a different plan for this:

wdyt?

beckermr commented 2 months ago

I am aware that making smaller PRs in the style I asked for is more work for the person making the PRs. On the other hand, the big PRs or reviewing subsets is more work for the reviewers.

The thing to remember is that I am not paid to do the review work here. While I like the new recipe format, I'm personally ok with it taking much longer to be used in conda-forge than the time scale you appear to want.

I very much appreciate all of the work being done by the prefix.dev folks on conda-forge and very much want to see that work continue.

We do however need to find a sustainable path to retooling conda-forge for the new recipe format. In a recent PR where I raised these concerns, we had another member of prefix do the review. I'm only marginally ok with this, we missed API breaks that should have been caught, and instead I would very much prefer you all to take a bit more time and care with how you ask for volunteer time through large PR review requests.

beckermr commented 2 months ago

As long as the PRs group tests + code and are around ~500 lines, I'm happy with them.

wolfv commented 2 months ago

I am closing this one since we merged all the items in multiple small PRs :)