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

fix clippy comments #500

Merged
merged 3 commits into from
Nov 29, 2023
Merged

fix clippy comments #500

merged 3 commits into from
Nov 29, 2023

Conversation

CGMossa
Copy link
Contributor

@CGMossa CGMossa commented Nov 27, 2023

@Enselic
Copy link
Collaborator

Enselic commented Nov 28, 2023

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

@CGMossa
Copy link
Contributor Author

CGMossa commented Nov 28, 2023

git rebase upstream master
git push --force-with-lease

I've done the above! Hopefully it works.

@Enselic
Copy link
Collaborator

Enselic commented Nov 29, 2023

The CI failure should be fixed by Enselic/cargo-public-api#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 added a commit to Enselic/syntect that referenced this pull request Nov 29, 2023
This version has fixed the bug that caused
trishume#500 to falsly fail.
@Enselic
Copy link
Collaborator

Enselic commented Nov 29, 2023

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

@CGMossa
Copy link
Contributor Author

CGMossa commented Nov 29, 2023

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
Copy link
Collaborator

Enselic commented Nov 29, 2023

Sorry for the mess... my fix was buggy, here is a fix of the fix: Enselic/cargo-public-api#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

  • it only works for organizations (or payed accounts)
  • I actually tested it during the beta but it was too buggy for my taste

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
Copy link
Contributor Author

CGMossa commented Nov 29, 2023

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.

src/parsing/syntax_set.rs Outdated Show resolved Hide resolved
@Enselic
Copy link
Collaborator

Enselic commented Nov 29, 2023

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

@CGMossa
Copy link
Contributor Author

CGMossa commented Nov 29, 2023

No worries. Let's gooooooooo! 😄

@CGMossa
Copy link
Contributor Author

CGMossa commented Nov 29, 2023

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
Copy link
Collaborator

Enselic commented Nov 29, 2023

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.

@Enselic Enselic merged commit 100b8b2 into trishume:master Nov 29, 2023
4 checks passed
@CGMossa CGMossa deleted the clippy_compliance branch November 29, 2023 15:56
@CGMossa
Copy link
Contributor Author

CGMossa commented Nov 29, 2023

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.

🤷

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.

None yet

2 participants