fix some pyrefly errors (part 1)

[pavelzw]

Checklist

• Added a news entry with any new deprecations added to the Deprecated section.
• Regenerated schema JSON if schema altered (python -m conda_smithy.schema)
• Regenerated linter documentation if any rules were added, removed or modified (python -m conda_smithy.linter.messages)

#2591 (comment)

this brings down the pyrefly errors from 223 to 204

my agent said the following

• Implemented the first “obvious fixes” batch.

  Changes made:

  - Removed dead Python 2 urllib fallback in bootstrap-obvious-ci-and-miniconda.py.
  - Fixed small uninitialized/local narrowing issues in token rotation and linter noarch checks.
  - Tightened simple signatures in ci_register, plugin, github, and validate_schema.
  - Made feedstock_io.get_repo() handle missing pygit2 and missing repos explicitly.
  - Corrected the datetime(...) construction in deprecation date arithmetic.
  - Fixed test helper and env-var preservation typing.

  Verification:

  - pixi run pyrefly ... now reports 203 errors, down from 222.
  - Focused pytest passed: 20 passed, 1 xfailed.
  - pixi run ruff check ... passed on touched files.
  - git diff --check passed.

done with codex but i looked over all fixes and made sure they make sense

[pavelzw]

this is needed, otherwise pixi run pyrefly check --python-interpreter-path $(pixi run which python) doesn't show all errors

[h-vetinari]

I'll admit I don't know pyrefly well, but I'm broadly in favour of using it (and potentially other type checkers) in CI; some of these fixes look a bit doubtful in terms of benefit, but nothing I'm actually opposed to. Thanks for the PR!

[h-vetinari]

I'll leave this open for a bit so other can chime in if desired

[h-vetinari]

@conda-forge/core, any thoughts towards adding more static checks here? If we go down this road, this PR will be one of many until we fix the >200 or so issues currently flagged by pyrefly (see #2591)

If there are no further comments, I'm planning to merge this in ~48h or so.

[isuruf]

@conda-forge/core, any thoughts towards adding more static checks here?

Unfortunately the first three files in the diff I looked at are false positives. Does adding more checks justify having to workaround false positives?

[h-vetinari]

I know what you mean; I noted above

some of these fixes look a bit doubtful in terms of benefit, but nothing I'm actually opposed to.

However, this is IMO a bit like formatting questions with black, i.e. not really worth arguing about, if someone thought about a structural approach and considers it worth linting.

pyrefly should help with topics like #2589, so that'd be an upside for which I could live with the opinionated-but-unnecessary changes. On the other hand, we could perhaps do this differently (e.g. mypy; tough I don't claim to know the differences between that and pyrefly).

There's also a possibility to create an exceptions file (see #2591, which has "passing" pyrefly checks despite not fixing anything), though this would currently be >2600 lines long. Since the those exceptions are very granular - e.g.

    {
      "line": 172,
      "column": 72,
      "stop_line": 172,
      "stop_column": 74,
      "path": "conda_smithy/anaconda_token_rotation.py",
      "code": -2,
      "name": "unbound-name",
      "description": "`gh` may be uninitialized",
      "concise_description": "`gh` may be uninitialized",
      "severity": "error"
    },

this sounds like something we'd have to keep up-to-date somewhat laboriously (though perhaps there's automation for that), so I'd rather just get rid of these errors; assuming we want to adopt pyrefly.

Perhaps you could open an issue about adding pyrefly (and/or about more type-checking in general) @pavelzw? It seems we might need more discussion about this that isn't really suited for a random PR.

[pavelzw]

I would feel more comfortable adding an autoupdater for py-rattler pinnings if we had type checking as mentioned in #2589 (comment)

In the current state of this repo, probably all occurrences of py-rattler are covered by unit tests(?) so we would catch issues in the PR anyway but if py-rattler usage grows, there might be some hidden branches that are not covered by the testing suite and latest then I think type checking is needed.

(I'm in general a fan of being able to see the types of something when working within my IDE, if this is properly supported in the project through type checkers, I'm all for it)

[pavelzw]

#2602

[pavelzw]

there has been no activity on the issue and py-rattler not being up to date is becoming an issue again (#2608). how should we proceed here?

as mentioned in #2589 (comment), this would help a lot with automatic updates for typed projects like py-rattler

[isuruf]

I'm against using (for lack of a better word) stupid tools. We end up battling the tool instead of writing code when we introduce these tools.

[baszalmstra]

I'm against using (for lack of a better word) stupid tools. We end up battling the tool instead of writing code when we introduce these tools.

I'm sorry, but this comment is not constructive at all. Can you clarify what you mean by "stupid tools"? Does pyrefly fall in that category? Are you against adding a type checker to this repository? Or this one in particular?

The reason this was proposed is that we want to pin versions of dependencies relatively strictly. But that adds the problem that the versions of these dependencies become stale quickly. The proposal was to make it easier to automatically bump versions of dependencies so that part of solving this issue can be automated.

Furthermore, we are having a hard time getting people to contribute to this repository. A type checker would help catch bugs earlier and faster and make the API of the code easier to navigate. This makes it easier for people to start contributing.

[isuruf]

Can you clarify what you mean by "stupid tools"?

Tools that reject certain coding styles just because the author of the tool does not like that coding style.

Does pyrefly fall in that category?

Yes, it does when I look at the diff of this PR.

Are you against adding a type checker to this repository?

Not at all. A type checker that doesn't limit legitimate code would be great to have.

[baszalmstra]

A type checker that doesn't limit legitimate code would be great to have.

Do you have one in mind? Or at least one that is more favorable? I'm just curious if we can make your criticism more actionable.

[isuruf]

Do you have one in mind? Or at least one that is more favorable?

No, I don't. I suggest you try a few type checkers and see which one doesn't reject legitimate code.

[baszalmstra]

Can you point out the parts that are legitimate and which the type checker should not reject? That would help!

[isuruf]

As I said above, the first three files in the diff are bad changes

[pavelzw]

outdated

i still do think that the runreqs_spacing change makes sense.

if this if block is not hit, you'll end up walking into a undefined variable that produces weird behavior in line 86.

other type checkers such as ty or mypy also flagged this when enabling --error possibly-unresolved-reference (or --check-untyped-defs in mypy's case) but i imagine they have similar odd behavior around other places tbh. they also all produce a similar amount of errors on this codebase so i don't really see a benefit of switching to a different type checker apart from giving me some more work to do

see d027420 (this PR)

[baszalmstra]

That means we’re kind of at a stalemate here. @isuruf, do you feel strongly enough about this to block the PR?

[lucascolley]

how about this for the third file?

diff --git a/conda_smithy/anaconda_token_rotation.py b/conda_smithy/anaconda_token_rotation.py
index bcd6091d..b1d0e035 100644
--- a/conda_smithy/anaconda_token_rotation.py
+++ b/conda_smithy/anaconda_token_rotation.py
@@ -62,10 +62,6 @@ def rotate_anaconda_token(

     anaconda_token = _get_anaconda_token()

-    gh = None
-    if github_actions:
-        gh = Github(gh_token())
-
     # capture stdout, stderr and suppress all exceptions so we don't
     # spill tokens
     failed = False
@@ -168,7 +164,7 @@ def rotate_anaconda_token(
                             raise RuntimeError(err_msg)

                 if github_actions:
-                    assert gh is not None
+                    gh = Github(gh_token())
                     try:
                         rotate_token_in_github_actions(
                             user, project, anaconda_token, token_name, gh

[pavelzw]

applied @lucascolley's suggestion

[pavelzw]

@isuruf ptal again

[pavelzw]

this is a minimal contributing.md partially stolen from #2519. eventually, i'd like to introduce a more reproducible pixi workflow to this repo, then this might also change.

[pavelzw]

as discussed in today's conda/conda-forge meeting with @isuruf.

will add this also to CI once all issues are fixed

[pavelzw]

i updated this file that you had issues with @isuruf, ptal again

[pavelzw]

i updated this change that you had issues with, ptal again @isuruf

[wolfv]

Thank you for working on this!

[jaimergp]

I wouldn't spend too much time on the scripts placed outside conda_smithy and tests. I don't think we using or testing them, which means we are not maintaining them either.

[pavelzw]

this was all codex, only some tokens were used

[lucascolley]

I don't think we using or testing them, which means we are not maintaining them either.

can we just delete them, then?

[jaimergp]

Probably, but not in this PR.

[lucascolley]

gh-2629

[jaimergp]

See #2628