Skip to content
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

Support skipping noarch selector lints #2090

Merged
merged 2 commits into from
Oct 11, 2024
Merged

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Oct 11, 2024

When building python recipes that builds a python version specific extensions and noarch python variants, the lint can be wrong. This gives a way for maintainers to skip that lint.

Checklist

  • Added a news entry
  • Regenerated schema JSON if schema altered (python conda_smithy/schema.py)

@isuruf isuruf requested a review from a team as a code owner October 11, 2024 16:58
When building python recipes that builds a python version specific
extensions and noarch python variants, the lint can be wrong.
This gives a way for maintainers to skip that lint.
@isuruf
Copy link
Member Author

isuruf commented Oct 11, 2024

cc @ocefpaf, @h-vetinari

@xhochy xhochy merged commit 59095a6 into conda-forge:main Oct 11, 2024
2 checks passed
@h-vetinari
Copy link
Member

This was merged a bit quickly IMO (e.g. missing news). The functionality is good, though I would have avoided the redundant naming, i.e. the whole block

linter:
  skip:
    - lint_noarch_selectors

is under linter:, so prefixing the individual skips with lint_ seems pointless (and somewhat inconsistent) to me

@isuruf
Copy link
Member Author

isuruf commented Oct 11, 2024

I was reading it as 'linter skip linting noarch selectors' and it seemed weird linter skip noarch selectors. What do you think?

I'll send a PR to add a news entry.

@h-vetinari
Copy link
Member

I was reading it as 'linter skip linting noarch selectors' and it seemed weird linter skip noarch selectors. What do you think?

To me, the linter can (by definition/nomenclature/implementation) only lint, so the verb in the skip command is implicit IMO. We cannot tell it to skip anything else but linting.

If we think about how this might be extended in the future (taking some possible examples, not actually proposing those), I think that

linter:
  skip:
    - noarch_selectors
    - hints
    - stdlib
    - license 
    - source_sha

makes more sense than

linter:
  skip:
    - lint_noarch_selectors
    - lint_hints  # <-- this one especially 
    - lint_stdlib
    - lint_license 
    - lint_source_sha

@isuruf
Copy link
Member Author

isuruf commented Oct 14, 2024

I was going to make a PR changing this, but it still felt weird to me.
I don't understand the lint_hints comment. I was thinking

linter:
  skip:
    - lint_noarch_selectors
    - hint_pip_check

@h-vetinari
Copy link
Member

It depends how granular we make the skips. Since they have no canonical names, I don't see another option than just documenting what skips we support and what they mean.

Why I thought about the hints is that - for example - a package may reasonably want to ignore the hint to replace matplotlib with matplotlib-base. But I wouldn't make the skip this granular, and just opt into skipping that part

for rq in build_reqs + host_reqs + run_reqs:
dep = rq.split(" ")[0].strip()
if dep in specific_hints and specific_hints[dep] not in hints:
hints.append(specific_hints[dep])

No idea what we would name that, I had thought about just "all hints", but fair enough if that's too big a hammer.

I was thinking

linter:
  skip:
    - lint_noarch_selectors
    - hint_pip_check

At least the separation into lint_ and hint_ is a point that makes sense to me (even though there are opposing arguments still). In any case, this is hopefully going to be rare usage, so I'll drop this discussion, and you can leave things as they are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants