FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Small changes to please cython-lint by GiacomoPope · Pull Request #211 · flintlib/python-flint · GitHub

  1. flintlib
  2. python-flint

Repository navigation

Small changes to please cython-lint - #211

Merged
oscarbenjamin merged 12 commits into
flintlib:mainfrom
GiacomoPope:linting_fixes
Sep 2, 2024
ConversationCommits (12)ChecksFiles changed
Merged

Small changes to please cython-lint#211
oscarbenjamin merged 12 commits into
flintlib:mainfrom
GiacomoPope:linting_fixes

Conversation

GiacomoPope commented Sep 2, 2024

Copy link
Copy Markdown
Contributor

Most changes are trivial.

See issue #210 about one remaining small change. The "last" thing I think we need to decide is a line length so we can fix E501 eventually.

GiacomoPope commented Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

I don't understand the E202 error in the CI as I don't get this locally and the error itself seems to point to f{x = } in format strings, which should be allowed... seems weird that this lint is failing for the CI machine but not locally on mine?!

GiacomoPope commented Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

Hmm it seems that people argue about this elsewhere: PyCQA/pycodestyle#1201 and if my E202 doesn't fail but it does on the CI I'll just include it back into the ignore.

oscarbenjamin reviewed Sep 2, 2024
View reviewed changes
Comment thread src/flint/types/acb_poly.pyx Outdated

def __pos__(self):
return self # ?
return self

oscarbenjamin Sep 2, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think something should be kept in the comment here.

This is not how it works for arb and acb since +a rounds to context precision:

In [1]: from flint import *

In [2]: a = arb(10**50)

In [3]: a
Out[3]: 1.00000000000000e+50

In [4]: +a
Out[4]: [1.00000000000000e+50 +/- 3.40e+34]

In [5]: a.man_exp()
Out[5]: (88817841970012523233890533447265625, 50)

In [7]: (+a).mid().man_exp()
Out[7]: (4814824860968089, 114)

I assume that the comment here is a reminder that maybe this method should be changed to match by using acb_poly_set_round.

GiacomoPope Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I have reintroduced the comments

oscarbenjamin reviewed Sep 2, 2024
View reviewed changes
Comment thread src/flint/types/fmpz.pyx Outdated
Comment on lines +530 to +532
# This is the correct code when fmpz_or is fixed (in flint 3.0.0)
#
#def __or__(self, other):

# def __or__(self, other):

oscarbenjamin Sep 2, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Now that Flint 3.0 is the minimum version we could use this code instead.

GiacomoPope Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ahh ok. Shall I make this change in this PR or another

oscarbenjamin Sep 2, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It should work fine just to uncomment this as it is tested. There was a bug in Flint 2.7 at the time.

GiacomoPope Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ok, this has been addressed

oscarbenjamin commented Sep 2, 2024

Copy link
Copy Markdown
Collaborator

seems weird that this lint is failing for the CI machine but not locally on mine?!

The two likely explanations for this are always:

  • Different version of something e.g. different cython-lint or perhaps even different Python version (some lint tools behave differently for different Python versions). Obvious thing is to make sure you have the latest version of cython-lint since that is what CI will use.
  • The PR branch is not based off of the tip of master i.e. there are further changes on master that are not in the PR branch. In this case a merge/rebase with master should mean that you see the same thing locally as in CI.

oscarbenjamin commented Sep 2, 2024

Copy link
Copy Markdown
Collaborator

Hmm it seems that people argue about this elsewhere: PyCQA/pycodestyle#1201

I think what happened there is that everyone agreed it was a bug apart from the pyflakes maintainer who did not want to discuss it and so closed and locked the issue.

Maybe we can do without this rule if it doesn't work properly.

GiacomoPope commented Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

At the moment the rule is flagging up the format strings (in the CI at least) which I disagree with (I think f"{x = }" produces a better output than f"{x=}"). Let me add some comments to the toml.

oscarbenjamin changed the base branch from master to main September 2, 2024 15:42

oscarbenjamin commented Sep 2, 2024

Copy link
Copy Markdown
Collaborator

At the moment the rule is flagging up the format strings (in the CI at least) which I disagree with (I think f"{x = }" produces a better output than f"{x=}")

The question is whether the rule is actually useful in other situations. If it is then we can work around it in this case e.g. f"x = {x}".

If the rule is generally not that important but a bit nice for formatting then I would be most inclined just to "fix" it throughout the codebase and then just leave it excluded in the lint config.

GiacomoPope commented Sep 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Looks like E202 is now fixed by doing the (in my opinion worse) option of x = {x} considering {x = } is intended to be used...

Maybe better to do this though than have the lint hack in place, as I don't know if } might come up in other places and be "bad" or whatever...

oscarbenjamin commented Sep 2, 2024

Copy link
Copy Markdown
Collaborator

Okay, looks good.

oscarbenjamin merged commit 397d973 into flintlib:main Sep 2, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

oscarbenjamin oscarbenjamin left review comments

Assignees

No one assigned

Labels

type: code quality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL