Skip to content

Conversation

@lbillinghamwrk
Copy link

@lbillinghamwrk lbillinghamwrk commented Jul 24, 2025

GhConf run https://github.com/optile/ghconf-runner/actions/runs/16501328390/job/46660454633
fails with NameError: name 'unicode' is not defined.

Unsurprisingly, as that hasn't been valid python syntax in more than 5.5 years.

That invalid syntax was in 2 functions both of which were redefinitions of earlier, valid callables.
This PR removes the invalid syntax, we should now have the valid callables in the method resolution order.

Goodness knows how this ever worked. Clearly there is no test coverage here.
We must either have never hit the {replace,remove}_team_push_restrictions functions before, or only hit them is circumstances where assert lines are swallowed (needs python compiler/interpreter to be compiled with unusual optimization flags)

Relates to PCPAY-3844 and https://app.incident.io/payoneer-checkout/incidents/362


we want this version
"""
assert all(isinstance(element, str) for element in teams), teams
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: Dear reviewer, you probably want to expand the diffs around the "we want this" so you can verify that the {replace,remove}_team_push_restrictions versions that we want to keep are indeed valid syntax

@lbillinghamwrk
Copy link
Author

thought: I tried running a linter with reasonable defaults over this codebase, there are a huge number of errors. However, none are F821 Undefined name or SyntaxError raising

@lbillinghamwrk lbillinghamwrk changed the title lolsob fix double method definitions and python2 syntax fix: 🐛 double method definitions and python2 syntax Jul 24, 2025
@lbillinghamwrk lbillinghamwrk force-pushed the wtaf-double-definitions-and-python2 branch from 770aa93 to d3e6460 Compare July 25, 2025 12:56
@lbillinghamwrk lbillinghamwrk force-pushed the wtaf-double-definitions-and-python2 branch from ebcf2a3 to ac9b96e Compare July 25, 2025 13:52
3.8: py38, docs, lint
3.9: py39
3.8: py38
3.9: py39, docs, lint
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: python 3.9 is what we use in ghconf-runner. That is only known current user of this code.

So we lint, docs, and typecheck on that

@lbillinghamwrk
Copy link
Author

lbillinghamwrk commented Jul 25, 2025

@jonathan-schmieg I think we should use your admin powers to force merge this PR.

There is no chance that I'm getting CI to pass for this fork.
The minimal change to get the test suite to start running is in #32, we are seeing tonnes of failures there for changes unrelated to cleaning up the python2-only NameErroring code.

GhConf is broken and I'm 99% sure that this PR will make it less broken if merged.

I guess previous maintainers of this fork just force merged rather than running PRs with check suites.

Especially since the last commit to the branch fixed completely invalid syntax that had been around for years
ce95301

@jonathan-schmieg jonathan-schmieg merged commit e2a9027 into optile-head Jul 25, 2025
0 of 3 checks passed
@jonathan-schmieg jonathan-schmieg deleted the wtaf-double-definitions-and-python2 branch July 25, 2025 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants