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

provide linter hints about adding stdlib, removing outdated ways to do it #1909

Merged
merged 2 commits into from
Apr 19, 2024

Conversation

h-vetinari
Copy link
Member

As discussed in core call today; I wanted to add this into run_conda_forge_specific originally (where the other hint logic from #1772 lives1, but it's a pain to run that in testing because run_conda_forge_specific requires a GH_TOKEN. If desired, I can still move it there, especially now that the tests have been debugged (and assuming CI here has a valid token). 🤷

Note: I'd also want to wait before merging releasing this until we have updated the stdlib docs in the knowledge base, to avoid people getting confused.

Footnotes

  1. which we cannot reuse because it currently only works for plain package names, no patterns, versions etc.

@beckermr
Copy link
Member

I didn't follow why a token was needed or w/e with the tests. However, LGTM!

@h-vetinari
Copy link
Member Author

I didn't follow why a token was needed or w/e with the tests.

Calling run_conda_forge_specific immediately runs into

def run_conda_forge_specific(meta, recipe_dir, lints, hints):
gh = github.Github(os.environ["GH_TOKEN"])

(and faking that token is no use because it is soon after used to fetch info from GH).

Hence testing run_conda_forge_specific needs a valid token, whereas "just" calling lintify_forge_yaml does not run into this constraint.

@beckermr
Copy link
Member

We could guard those lines and calls to enable things to be run easily without tokens.

@h-vetinari
Copy link
Member Author

We could guard those lines and calls to enable things to be run easily without tokens.

Sure. Not sure though if it's worth the effort? For now it seems to work OK as is and doesn't seem tooooo ugly? (then again, perhaps I also haven't understood where exactly the split between run_conda_forge_specific and lintify_forge_yaml should be). How about a follow-up issue?

@h-vetinari h-vetinari marked this pull request as ready for review April 19, 2024 22:26
@h-vetinari h-vetinari requested a review from a team as a code owner April 19, 2024 22:26
@h-vetinari
Copy link
Member Author

OK, now that we have some basic documentation for stdlib up, let's put this one in. :)

@h-vetinari h-vetinari merged commit 8a2eba4 into conda-forge:main Apr 19, 2024
2 checks passed
@h-vetinari h-vetinari deleted the stdlib_lint branch April 19, 2024 22:27
@ytausch
Copy link
Contributor

ytausch commented Apr 26, 2024

@h-vetinari @beckermr I suggest not introducing any new code that uses io.open. In Python 3, the preferred way should be just open, which is equivalent to io.open.

@jakirkham
Copy link
Member

Maybe someone can add an appropriate ruff configuration to the linters here

@h-vetinari
Copy link
Member Author

I suggest not introducing any new code that uses io.open. In Python 3, the preferred way should be just open, which is equivalent to io.open.

Yeah, completely fine by me, I just happened to overlook this while C&P-ing from pre-existing smithy code.

@jakirkham jakirkham mentioned this pull request Apr 29, 2024
@ytausch
Copy link
Contributor

ytausch commented May 3, 2024

Maybe someone can add an appropriate ruff configuration to the linters here

Done. See #1919

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.

4 participants