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

set MSRV (to 1.53) #195

Merged
merged 1 commit into from
Aug 3, 2021
Merged

set MSRV (to 1.53) #195

merged 1 commit into from
Aug 3, 2021

Conversation

tshepang
Copy link
Contributor

@tshepang tshepang commented Aug 1, 2021

No description provided.

@tshepang
Copy link
Contributor Author

tshepang commented Aug 1, 2021

I included testing on macOS and Windows, for I was curious

README.md Outdated
Comment on lines 23 to 24
The minimum supported Rust toolchain is v1.53.0,
due to usage of or-patterns syntax.
Copy link
Member

@lukechu10 lukechu10 Aug 1, 2021

Choose a reason for hiding this comment

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

I don't think we should mention why it's 1.53.0. We might use other features later and this would be outdated.

Suggested change
The minimum supported Rust toolchain is v1.53.0,
due to usage of or-patterns syntax.
The minimum supported Rust toolchain is v1.53.0. Sycamore is not guaranteed to compile on an older version of Rust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumping MSRV is an explicit (and somewhat rare) thing, and one that does it would know why, and would not miss updating this. An alternative is to only care about supporting latest stable, and that's when I think the reason is not needed.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I wasn't being clear here. What I meant was that or-patterns might not be the only 1.53 feature we use. I seem to recall that RefCell::take and a few other functions were also stabilized recently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only error I get when compiling with 1.52 is about or-patterns.

Copy link
Member

Choose a reason for hiding this comment

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

But setting our MSRV to 1.53.0 does not mean that or-patterns is the only feature we are allowed to use. And technically, we could remove or-patterns from the code base to make it compatible with older versions of Rust. It's not like it's as crucial as some other features like specialization or fn_traits.

@lukechu10
Copy link
Member

What I meant about trybuild in the issue was that rustc error messages tend to vary a lot between releases. Therefore the ui tests should only be run on 1.53.0

A simple fix for this would be to add the following to the tests in packages/sycamore-macro/test/template_macro.rs and packages/sycamore-router-macro/router-macros.rs:

#[test]
fn ui() {
    if std::env::var("RUN_UI_TESTS").is_ok() {
        // stuff here
    }
}

And only set the environment variable when matrix.os == 1.53.0

@tshepang tshepang force-pushed the 191 branch 5 times, most recently from a32da6d to 5e0554c Compare August 2, 2021 18:24
@tshepang tshepang marked this pull request as ready for review August 2, 2021 18:49
Also, run UI test only on a single compiler version,
because rustc error messages tend to vary between releases.

Closes sycamore-rs#191
@lukechu10
Copy link
Member

Awesome. Thanks!

@lukechu10 lukechu10 merged commit 8dfd807 into sycamore-rs:master Aug 3, 2021
@tshepang tshepang deleted the 191 branch August 3, 2021 03:17
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.

2 participants