Skip to content

track: Migrated the existing exercises to the Rust 2018 edition#769

Merged
petertseng merged 4 commits intoexercism:masterfrom
ZapAnton:2018_edition
Dec 17, 2018
Merged

track: Migrated the existing exercises to the Rust 2018 edition#769
petertseng merged 4 commits intoexercism:masterfrom
ZapAnton:2018_edition

Conversation

@ZapAnton
Copy link
Copy Markdown
Contributor

@ZapAnton ZapAnton commented Dec 9, 2018

This PR shows what simple migration to the 2018 edition would look, when cargo fix is applied to every exercise on the track.

Since the changes were nor discussed nor approved by the maintainers , this PR serves as a demonstration of the migration, and should be closed, if the changes are not desirable.

A script, that was used for the migration:

#!/usr/bin/env bash                                                                                                                               
                                                                                                                                                  
for dir in exercises/*/; do                                                                                                                       
    (                                                                                                                                             
        cd $dir                                                                                                                                   
                                                                                                                                                  
        sed -i '2iedition = "2018"' Cargo.toml                                                                                                    
                                                                                                                                                  
        cargo fix --edition-idioms --allow-dirty                                                                                                  
                                                                                                                                                  
        cargo fmt                                                                                                                                 
                                                                                                                                                  
        cargo clean                                                                                                                               
    )                                                                                                                                             
done    

Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

I imagine students will appreciate being able to use 2018. I seem to recall there was an example solution I wrote recently where I would have benefited from a non-lexical lifetime.

I would like to see the cargo fmt results in a different commit, because they are not related to moving editions. I'd like to also take this opportunity to remind interested individuals who might have an opinion on #659 .

cargo fix probably can go in the same commit as addition edition however.

@ZapAnton
Copy link
Copy Markdown
Contributor Author

Done

Copy link
Copy Markdown
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Approved pending getting Travis to pass. Haven't looked yet into what's causing that failure.

One interesting exercise which I have not yet performed: with a pre-rust-2018 compiler, attempt to independently compile all stubs and examples. I intuit that all stubs should compile, but that several examples will not. However, I assert that it is acceptable to impose a minimum rustc version for this track, given the ease of upgrading with rustup.

@ZapAnton
Copy link
Copy Markdown
Contributor Author

ZapAnton commented Dec 10, 2018

The build failed for several reasons:

  • The READMEs for some exercises are not up-to-date. Since simple-cipher, two-fer: READMEs from problem-specifications #771 was merge the rebase will solve this.
  • Some deprecated exercises (namely nucleotide-codons and hexadecimal) were modified and included in the Travis check. Since they are deprecated and do not even have a stub file, they could be ignored.
  • dominoes and reverse-string have some actual errors in them. Will investigate further

@ZapAnton
Copy link
Copy Markdown
Contributor Author

Some deprecated exercises (namely nucleotide-codons and hexadecimal) were modified and included in the Travis check.

Actually that raises a question - should deprecated exercises be present on the track? Maybe they could be removed, so not to complicate Travis scripts?

@coriolinus
Copy link
Copy Markdown
Member

coriolinus commented Dec 10, 2018 via email

@ZapAnton
Copy link
Copy Markdown
Contributor Author

OK, now Travis has failed only because of the deprecated exercises. To make it pass I would have to rebase against #773

@petertseng petertseng force-pushed the 2018_edition branch 2 times, most recently from 75e43fe to 35a42c5 Compare December 17, 2018 01:32
@petertseng petertseng force-pushed the 2018_edition branch 4 times, most recently from ba2a443 to 6752176 Compare December 17, 2018 03:42
@petertseng
Copy link
Copy Markdown
Member

Note: dfdb48f should be good

@petertseng
Copy link
Copy Markdown
Member

Trying with just the edition and use path changes to see what's required to build

@petertseng
Copy link
Copy Markdown
Member

Okay, that build succeeded, so only the use path changes are required. Will leave notes in commit messages explaining that.

For the sake of making this more easily reviewable, this limits the
changes to only the Cargo*.toml files. However, a `use` change will be
required to compile:

https://rust-lang-nursery.github.io/edition-guide/rust-2018/module-system/path-clarity.html

Thus, we intentionally break bisectability for the sake of
reviewability.
@petertseng petertseng force-pushed the 2018_edition branch 2 times, most recently from b988195 to 065ceff Compare December 17, 2018 04:30
Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

I decided that the cargo fix does instead go in a separate commit, since I discovered it is not required for compilation

@petertseng
Copy link
Copy Markdown
Member

Other changes I made to support this PR:

In Rust 2018, paths in `use` declarations must begin with a crate name,
`crate`, `self`, or `super`.

Required for the affected exercises to compile under Rust 2018

https://rust-lang-nursery.github.io/edition-guide/rust-2018/module-system/path-clarity.html
None of these are required to compile any exercise, but it's useful to
be in line with 2018 idioms.

Most changes are of the form: `extern crate` becomes `use`.

Note an addition of an unnamed lifetime in roman-numeral.
@petertseng petertseng merged commit 9846f36 into exercism:master Dec 17, 2018
@ZapAnton ZapAnton deleted the 2018_edition branch December 17, 2018 08:32
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