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

Add pure Rust versions of dump loading and creation #84

Merged
merged 1 commit into from
Jun 30, 2017
Merged

Conversation

trishume
Copy link
Owner

Splits out the existing functionality into dump-load and dump-create
features and adds dump-load-rs and dump-create-rs features which use
the libflate crate but are respectively slower and worse at compressing.

Also updates the LICENSE.txt file to comply with Google's preferences
and updates .travis.yml to build the configuration used by Xi.

cc @Byron since you don't use default features so you'll probably have to add the dump features when upgrading.

Splits out the existing functionality into dump-load and dump-create
features and adds dump-load-rs and dump-create-rs features which use
the libflate crate but are respectively slower and worse at compressing.

Also updates the LICENSE.txt file to comply with Google's preferences
and updates .travis.yml to build the configuration used by Xi.
@trishume trishume merged commit 5a03606 into master Jun 30, 2017
@trishume trishume deleted the rust-dumps branch June 30, 2017 17:19
@Byron
Copy link
Contributor

Byron commented Jul 1, 2017

Thanks a lot for letting me know! The upgrade worked just fine after adding the feature you suggested.

@boxofrox
Copy link

boxofrox commented Jul 2, 2017

@trishume What are your thoughts regarding semver compliance?

I ask from the perspective of a user that relies on cargo install as a package manager of sorts for programs. If those programs were written against v1.5 of syntect without the default features, they will no longer build via cargo install because the API has changed.

The only alternative I see to prevent this situation would be to release this PR as v2.0.

@trishume
Copy link
Owner Author

trishume commented Jul 3, 2017

@boxofrox my thoughts are I constantly feel guilty for breaking everything, but the alternative is to have every single release be a major release even though it doesn't break the vast majority of clients. I purposefully chose to expose almost all internals of syntect as a public interface so that power users can use the fancy features. I planned to only bump the major version number when I changed the basic syntax highlighting interface.

I try to work around that by pinging people that I know will have to change their code, but every time I do this I find out I broke someone I didn't know used the power user feature.

I'll probably to continue to want to split things in the default build out into features, unfortunately Cargo doesn't give me a way to do that without breaking backward compatibility with people who don't use default features. I could use add a no-something feature, but that doesn't work when I also want to make a crate an optional dependency, which can only be done by adding a feature.

I'm willing to change my behaviour given how much trouble it has caused. Do you think I should start having maybe 50% of versions be a major revision bump, or should I just continue what I'm doing but also ping you when I break non-default features.

@boxofrox
Copy link

boxofrox commented Jul 3, 2017

@trishume, thanks for sharing your thoughts. You're welcome to ping me, though I'd just ping @epage with a cobalt-rs issue/PR in turn as I'm a new user of the project.

...the alternative is to have every single release be a major release even though it doesn't break the vast majority of clients. I purposefully chose to expose almost all internals of syntect as a public interface so that power users can use the fancy features.

I had not considered this perspective, and it makes sense that such an API would be difficult to maintain. With this understanding, cobalt-rs can use version = "^1.5.0" instead of ^1.5 to avoid this incident.

I'll probably to continue to want to split things in the default build out into features, unfortunately Cargo doesn't give me a way to do that without breaking backward compatibility with people who [don't?] use default features.

True. In the case of this PR, the only point of concern for me is the potential for some abandoned binary crate to become rendered useless when a dependency doesn't follow strict adherence to semver. The concern is of little consequence here, though, because I can submit a PR on the cobalt-rs side, and we'll clear it up.

As I continue learning Rust, I'll be more aware when considering crates or how I select a version.

I'm willing to change my behaviour given how much trouble it has caused. Do you think I should start having maybe 50% of versions be a major revision bump, or should I just continue what I'm doing but also ping you when I break non-default features.

I don't think I have enough invested to recommend changing your course of action. I was merely curious, and should I find myself maintaining a crate, I will have a better understanding of the difficulties that entails.

It may be prudent to recommend in the README that non-default "users" bind to the patch version (e.g. syntect = "1.7.0"), since it's possible for the API to break despite best efforts.

Cheers!

@trishume
Copy link
Owner Author

trishume commented Jul 3, 2017

@boxofrox thanks for the input. I like the idea of recommending users of no-default-features bind to the patch revision. Of course all the existing users don't follow that, so maybe next time I make a breaking change I'll bump to 2.0.0 and add comments/docs to all "advanced" features to bind to the patch revision.

@epage
Copy link

epage commented Jul 4, 2017

First off, thanks for your work on this project!

I'm one of the maintainers of cobalt which is where boxofrox ran into this problem.

I disabled default features because syntect broke on me mysteriously when integrating serde, as in errors internal to syntect about functions expecting one map type but being given a different one. I didn't open an issue because I didn't know if we "were holding it wrong". I realize now that I should have because it'd at least lead to a documentation improvement.

So in my rush to get my feature in, I saw that syntect was composable and that the problem I was having was in a part of syntect that we didn't need. I saw nothing in the documentation suggesting disabling default features is unstable. I was not the original implementer of our use of syntect, so I have no clue if the documentation marks parts of the API as unstable.

I think its an interesting balance of trying to open the door for power users while not being tied down with compatibility.

Some ideas I have to improve the situation

  • If they aren't already, mark the unstable functions as such
  • Suffix the default features with _unstable to let people know touching them is risky. Maybe then have a feature without an _unstable suffix that depends on all the others and that is what is the default feature.

or

  • Split the implementation details out into a separate pre-1.0 crate and have the current crate call into it. This is somewhat similar to the idea of regex and regex-syntax.

@trishume
Copy link
Owner Author

trishume commented Jul 4, 2017

@epage that's weird about serde breaking things. That was somewhat expected if you were using custom dumps that aren't the internal ones, because serde changed the format and I didn't include good detection of non-matching formats. That was another breaking change that I did that didn't conform to semver because I thought I knew every user of custom dumps, but didn't.

I don't think I'll go with function names with _unstable in them, since I don't like the aesthetics. I might try the two crates thing actually.

I'll think about it next time I want to break something. Whatever I do, next time I won't ignore semver, this is now the third or fourth time me doing so has caused problems.

@epage
Copy link

epage commented Jul 4, 2017

@epage that's weird about serde breaking things. That was somewhat expected if you were using custom dumps that aren't the internal ones, because serde changed the format and I didn't include good detection of non-matching formats. That was another breaking change that I did that didn't conform to semver because I thought I knew every user of custom dumps, but didn't.

I really have no clue what was happening since I wasn't too familiar with how the syntect stuff was working in cobalt. The error was in syntect code calling syntect code, as far as I could tell. We had all the default features on. I saw nothing to allow the customization of the map data type. I was baffled, found a work around, and moved on.

I don't think I'll go with function names with _unstable in them, since I don't like the aesthetics. I might try the two crates thing actually.

I don't know if people normally suffix their functions with _unstable (I was only recommending that for features which I have seen). My comment was intentionally vague. I know in the stdlib, I've seen some kind of comment in the docs about the function being unstable.

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.

4 participants