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

Stop requiring a bleeding-edge Cython. #206

Merged
merged 3 commits into from
Mar 4, 2023

Conversation

alexmv
Copy link
Contributor

@alexmv alexmv commented Mar 2, 2023

Fixes: #204. See #205 for an alternate fix, which I don't prefer.

@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Merging #206 (152c0d4) into main (6ee9f63) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #206   +/-   ##
=======================================
  Coverage   49.24%   49.24%           
=======================================
  Files           4        4           
  Lines         266      266           
  Branches       39       39           
=======================================
  Hits          131      131           
  Misses        121      121           
  Partials       14       14           
Impacted Files Coverage Δ
line_profiler/line_profiler.py 50.26% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ee9f63...152c0d4. Read the comment docs.

@Theelx
Copy link
Collaborator

Theelx commented Mar 2, 2023

Based on the fact that 3.0.0b1's compiler directive would require us to detect the cython version at buildtime, which is annoying, I can back this PR as a temporary fix until 3.0.0 stabilizes.

@alexmv
Copy link
Contributor Author

alexmv commented Mar 3, 2023

For any future visitors to this PR, most of the discussion was on #205.

@Erotemic
Copy link
Member

Erotemic commented Mar 3, 2023

Yikes, it looks like you have some auto-formatting enabled. That makes the diff a lot more complex than it needs to be. Is there any way to revert that?

I absolutely open to formatting PRs, but it would be good if it was separated into a different PR. I also I have strong opinions on quotes. Particularly I believe:

* Single quotes (') are used for code
* Double quotes (") are used for documentation
* Don't touch any string that has an internal quote.

I also do have an auto-formatter that accomplishes this.

I'm fine with the changes to the docstring spacing, import sorting, and getting rid of parens around the yeild, but the extra new lines in the argument parser and other function calls and switching to double quotes is something I'd rather not have.

@alexmv
Copy link
Contributor Author

alexmv commented Mar 3, 2023

Yikes, it looks like you have some auto-formatting enabled. That makes the diff a lot more complex than it needs to be. Is there any way to revert that?

Yikes indeed! Sorry, that was absolutely unintentional -- I have a hook to apply black, which fired. I've repushed with those formatting changes dropped.

pyproject.toml Outdated Show resolved Hide resolved
Requiring `Cython>=3.0.0a11` opens the door to any later pre-release;
this includes the newly-released 3.0.0b1, which breaks backwards
compatibility or requires the addition of a non-backwards-compatible
compiler flag (`legacy_implicit_noexcept`).  Fixing this would require
code changes and pinning the minimum Cython version to 3.0.0b1.

There is no reason to require a bleeding-edge version of Cython.
Doing so opens the door to installs suddenly failing due to breaking
changes in dependencies, such as this one.

Python 3.12 will break compatibility with Cython 0.29, however, so we
make an exception for that version of Python.  For Python 3.12, we
leave Cython pinned to 3.0.0a11, which is the latest version which
works with that Python but is still compatible with the current code.

For other versions of Python, revert the version bump to bleeding-edge
Cython added in a9448ec, and only require
0.29.24 or higher.

Fixes: pyutils#204.
@Erotemic
Copy link
Member

Erotemic commented Mar 4, 2023

Thanks @alexmv @andersk @Theelx ! Merging and releasing.

@Erotemic Erotemic merged commit 6ffb10d into pyutils:main Mar 4, 2023
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.

pip install fails however conda install succeeds
4 participants