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

Reduce dependencies and #[macro_use] usage #438

Merged
merged 3 commits into from
Jul 12, 2022
Merged

Reduce dependencies and #[macro_use] usage #438

merged 3 commits into from
Jul 12, 2022

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Jun 23, 2022

No description provided.

Instead use once_cell::sync::Lazy.
… removes the direct dependency on serde_derive.
@Enselic
Copy link
Collaborator

Enselic commented Jun 24, 2022

Thumbs up for working on getting rid of lazy_static 👍 I have been wanting to do that myself, but I have been held back by the fact that it changes the public API in a semver breaking way (and I did not have enough energy to sort that out for 5.0.0). You can run this command to see the API diff:

% cargo install cargo-public-api
% cargo public-api --diff-git-checkouts origin/master macro-use
Removed items from the public API
=================================
-pub fn syntect::parsing::SCOPE_REPO::deref(&self) -> &Mutex<ScopeRepository>
-pub type syntect::parsing::SCOPE_REPO::Target = Mutex<ScopeRepository>

Changed items in the public API
===============================
-pub struct syntect::parsing::SCOPE_REPO
+pub static syntect::parsing::SCOPE_REPO: Lazy<Mutex<ScopeRepository>>

Added items to the public API
=============================
(none)

Now, the question is if anyone actually uses SCOPE_REPO directly. We could download all syntect dependents using a similar script as in sharkdp/bat#2222 and see. If it is not used in any dependents on crates.io, we could try to get away with changing the public API anyway, like we did for ContextId::new(). In the worst case, peoples builds stop working, and we can yank the published version, revert this PR, and do another release.

Another option could be to somehow try to keep he current API but mark it as deprecated. Not sure if that is doable. Especially not if we also want to get rid of lazy_static right away.

I haven't reviewed your code in detail, but maybe you want to split this PR up into smaller PRs so the other changes are not blocked by the lazy_static challenges?

I also notice that MSRV CI is failling. Looks like the time crate requires us to bump MSRV again. I haven't looked in detail at that either though.

@jplatte
Copy link
Contributor Author

jplatte commented Jun 27, 2022

Oh, I didn't notice that these were part of the public API. I don't really care how it's solved 🤷🏼

I could look into what code lazy_static actually generates and see whether that could be 'emulated' with once_cell, but not any time soon.

@Enselic
Copy link
Collaborator

Enselic commented Jul 1, 2022

I analyzed all current syntect dependents on crates.io and found no single use of SCOPE_REPO. I think we should be brave and flat out remove it from the API without a major semver bump. It is worth noting that its docs say This shouldn't be necessary for you to use.

We then just have to keep our eyes open when we make the next release, and if there is unexpected and wide breakage, we yank, revert, and republish. Not a super big deal.

Does that sound OK to you @trishume ? I can take care of all the chores of doing that if that is OK with you.

The script I used

mkdir syntect-dependents
cd syntect-dependents
#/usr/bin/env bash

set -o xtrace

dependents=$(for page in $(seq 1 10); do
    curl -L "https://crates.io/api/v1/crates/syntect/reverse_dependencies?page=$page" | jq --raw-output '.versions | map(.crate) | flatten[]'
    sleep 2 # avoid throttling
done)

echo $dependents

for crate in $dependents; do
    echo "Processing $crate"
    mkdir -p "$crate"
    cd "$crate"
    newest_version=$(curl -L https://crates.io/api/v1/crates/$crate | jq --raw-output .crate.newest_version)
    echo "$newest_version"
    curl -L "https://crates.io/api/v1/crates/$crate/$newest_version/download" | tar -zxf -
    cd ..

    sleep 2 # avoid throttling
done

and then

grep -r SCOPE_REPO *

gives no hits.

@Enselic
Copy link
Collaborator

Enselic commented Jul 3, 2022

Maybe it would be better to be less radical and at least first make a release with SCOPE_REPO marked as deprecated...

Another option could be to go straight for 6.0.0, but even in that case it probably would be good manners to first make a deprecation...

Copy link
Owner

@trishume trishume left a comment

Choose a reason for hiding this comment

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

I suspect actually nobody uses that feature, especially given your checking. Especially since it's only for optimizing advanced text editors. Happy to make the call to just break it without a major version bump. If anyone has their stuff broken they can come to this comment and disapprove of my recklessness.

@trishume trishume merged commit 606450b into trishume:master Jul 12, 2022
@jplatte jplatte deleted the macro-use branch July 12, 2022 08:13
@Enselic Enselic mentioned this pull request Jul 30, 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.

3 participants