-
Notifications
You must be signed in to change notification settings - Fork 4
fix: 🐛 double method definitions and python2 syntax #31
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
fix: 🐛 double method definitions and python2 syntax #31
Conversation
|
|
||
| we want this version | ||
| """ | ||
| assert all(isinstance(element, str) for element in teams), teams |
There was a problem hiding this comment.
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
|
thought: I tried running a linter with reasonable defaults over this codebase, there are a huge number of errors. However, none are |
770aa93 to
d3e6460
Compare
ebcf2a3 to
ac9b96e
Compare
| 3.8: py38, docs, lint | ||
| 3.9: py39 | ||
| 3.8: py38 | ||
| 3.9: py39, docs, lint |
There was a problem hiding this comment.
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
|
@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. 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 |
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_restrictionsfunctions before, or only hit them is circumstances whereassertlines 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