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

Use crates.io unicode_names2 0.6.0 #6478

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Conversation

konstin
Copy link
Member

@konstin konstin commented Aug 10, 2023

Update unicode_names2 to the crates.io release 0.6.0, removing a git dependency.

@youknowone As far as i can tell, all changes from https://github.com/youknowone/unicode_names2 have been merged into https://github.com/progval/unicode_names2, is it correct that all changes needed for the RustPython parser are in the 0.6.0 release on crates.io?

@konstin konstin added the internal An internal refactor or improvement label Aug 10, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 10, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Cool with this assuming it contains all the changes.

@youknowone
Copy link
Contributor

I wasn't following up this issue for a while.
As I understand, this patch is required to keep compatibility. progval/unicode_names2#12
But let me look them in again for next a few weeks.
I was not able to working on open source projects for a while due to other stuff. it will takes a few more weeks.

@charliermarsh
Copy link
Member

Ah yeah, I think upstream is still missing some changes that we need.

@konstin
Copy link
Member Author

konstin commented Aug 21, 2023

i think we need progval/unicode_names2#12, but i don't understand how since none of our tests are failing without it. Upstream is maintained and has just published a 1.0.0, so i'm even more motivated to migrate

@charliermarsh
Copy link
Member

I think this shows a failing test case: RustPython/RustPython#4566

@charliermarsh
Copy link
Member

Unless it no longer fails after switching to the main channel, of course.

@konstin
Copy link
Member Author

konstin commented Aug 21, 2023

thanks for link! I added the test, it seems that this is already passing.

@konstin konstin marked this pull request as ready for review August 21, 2023 20:44
@charliermarsh
Copy link
Member

As annoying as it is... I'd really love to understand why it works now (and didn't before) prior to merging. Does that snippet fail on 0.6.0 (non-Git, like before we pinned)?

@konstin
Copy link
Member Author

konstin commented Aug 21, 2023

oh, i realized i actually updated only one of two unicode_names2 usages. I updated the other too, and now the test fails as expected.

@konstin konstin marked this pull request as draft August 21, 2023 20:58
@manmartgarc
Copy link
Contributor

Hey, just checking in on this. I was able to use a temporary LibCST crates on #7179. As far as I know unicode_names2 is the only other git dependency left to vanquish. What is left to do to merge this change?

@konstin
Copy link
Member Author

konstin commented Oct 2, 2023

I've checked and the relevant PR was merged in the mean time (progval/unicode_names2#12). I've asked for a crates.io release: progval/unicode_names2#28

@charliermarsh
Copy link
Member

Thanks @manmartgarc. By the way: I temporarily moved LibCST back to a Git dependency so that we could get out Python 3.12 support today, but I'll move it back to crates as soon as they publish a new version.

@konstin
Copy link
Member Author

konstin commented Oct 2, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@konstin konstin marked this pull request as ready for review October 2, 2023 21:19
@konstin
Copy link
Member Author

konstin commented Oct 2, 2023

The maintainer reacted really quickly, this is now ready to be merged

@@ -25,7 +25,7 @@ insta = { version = "1.33.0", feature = ["filters", "glob"] }
is-macro = { version = "0.3.0" }
itertools = { version = "0.11.0" }
log = { version = "0.4.17" }
memchr = "2.6.4"
memchr = { version = "2.6.4" }
Copy link
Member Author

Choose a reason for hiding this comment

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

Made this consistent while i was at it

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Great!

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 2, 2023

CodSpeed Performance Report

Merging #6478 will degrade performances by 3.69%

Comparing unicode_names2_0-6-0 (7ab46fc) with main (c6d0bdd)

Summary

❌ 1 (👁 1) regressions
✅ 24 untouched benchmarks

Benchmarks breakdown

Benchmark main unicode_names2_0-6-0 Change
👁 linter/default-rules[large/dataset.py] 92.5 ms 96 ms -3.69%

@charliermarsh charliermarsh merged commit 3ccd1d5 into main Oct 2, 2023
16 of 31 checks passed
@charliermarsh charliermarsh deleted the unicode_names2_0-6-0 branch October 2, 2023 22:17
@youknowone
Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants