trishume / syntect

Rust library for syntax highlighting using Sublime Text syntax definitions.
https://docs.rs/syntect
MIT License
1.89k stars 132 forks source link

fix `clippy` comments #500

Closed CGMossa closed 10 months ago

CGMossa commented 10 months ago

While working on another PR #499 , I ran cargo clippy to get familiar with the code-base.

Depends on

Enselic commented 10 months ago

Thanks! Can you rebase on latest master so CI passes please?

CGMossa commented 10 months ago
git rebase upstream master
git push --force-with-lease

I've done the above! Hopefully it works.

Enselic commented 10 months ago

The CI failure should be fixed by https://github.com/Enselic/cargo-public-api/pull/516. The plan is:

  1. The above fix is merged and a new version of the crate is released.
  2. I upgrade syntect to the new version
  3. We rebase this PR and then it should pass CI :)

Sorry for the inconvenience!

Enselic commented 10 months ago

Can you rebase again please? Now it should work :)

CGMossa commented 10 months ago

Done! I can't make CI run again unfortunately.

I'd also maybe advertise https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

I don't know about it myself, but it might help with the call to "rebase". Some folks have had success using it.

Enselic commented 10 months ago

Sorry for the mess... my fix was buggy, here is a fix of the fix: https://github.com/Enselic/cargo-public-api/pull/518

This time I have also confirmed that the public_api test passes for this PR with the above fix.

Thanks for the tip with the merge queue, but

This repo is pretty low traffic, but if it becomes high traffic it could make sense to look into something that implements the It's Not Rocket Science Rule.

CGMossa commented 10 months ago

No worries. I also learned something new here. I've added a "Depends on" in the description, so we'll know when the time is right.

Enselic commented 10 months ago

It feels awkward and embarrassing to ask, but can you rebase again please?

CGMossa commented 10 months ago

No worries. Let's gooooooooo! 😄

CGMossa commented 10 months ago

Could I ask you to make a release to crates-io of syntect at some point :D Selfishly, I pushed for #499 in order to upgrade typst :P packages

Enselic commented 10 months ago

Thanks, let's merge!

Regarding making a release, right now what's on master requires us to release 6.0.0, which I'd like to avoid. So we should probably revert the bump that changed the API in an backwards incompatible way. Better yet would probably be to remove external types from the public API so we have full control over when we change the API. But that could be a big and invasive changed, maybe not even doable.

If we revert bitflags and then go over all other deps that can be bumped, I think we can make a 5.x release to crates-io. Maybe you are interested in going over and bumping deps? Not minor bumps, just x.0.0 bumps and 0.x.0 bumps, since they prevent a single version of a dep to be used by dependents, if the dependents depend on other versions of dependencies.

CGMossa commented 10 months ago

Hmm.. I'm not an expert honestly. I've tried bumping "plist", in #504 and I've got an error. Plus I tried to revert

git revert e9819fb                                       
error: commit e9819fbcaf0eb5ea73448079aac78d00b8b4d140 is a merge but no -m option was given.        
fatal: revert failed

which should be the merge commit for the PR you mentioned.

🤷