Skip to content

feat: add fake manifest for analyzers/IDEs (fixes #443) #448

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

Closed
wants to merge 2 commits into from

Conversation

jrvidal
Copy link
Contributor

@jrvidal jrvidal commented Jun 21, 2020

This enables analyzers to work correctly as if the exercises folder were a true Cargo project.

  • It is an "orphan" Cargo.toml, completely unrelated to the root Cargo.toml. This has pros and cons.
    • It could be potentially confusing to learners, since they have to open the exercises folder for the analyzer to work.
    • OTOH it does not "infect" the main project build with tons of targets, many of them in a non-compilable state.
  • The folder structure is unchanged! 🎉

@@ -0,0 +1,286 @@
# This Cargo.toml is AUTOGENERATED, you shouldn't need to edit it.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could generate this on the fly, but it does seem a bit more error-prone

src/exercise.rs Outdated
@@ -167,9 +167,10 @@ path = "{}.rs""#,
fn run(&self) -> Result<ExerciseOutput, ExerciseOutput> {
let arg = match self.mode {
Mode::Test => "--show-output",
_ => ""
_ => "",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some cargo fmt noise here and there.

@@ -0,0 +1,11 @@
[package]
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 maintenance commands are hidden behind a lib and a feature so it does not interfere with the normal build. It is also exposed as an alias in .cargo/config to help prospective contributors.

authors = ['The Rustlings Maintainers']
publish = false

[[bin]]
Copy link
Contributor Author

@jrvidal jrvidal Jun 21, 2020

Choose a reason for hiding this comment

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

All targets are configured as [[bin]]. I'm not sure if this could be problematic for exercises without main (maybe some analyzers will complain that is missing?).

We can't have multiple lib targets though, so if [[bin]] doesn't work we might need to get creative with [[examples]] and others.

Copy link
Contributor

Choose a reason for hiding this comment

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

Errors are produced for each target without a main fn.

@manyinsects manyinsects self-requested a review July 8, 2020 09:51
@manyinsects manyinsects self-assigned this Jul 8, 2020
@jsejcksn
Copy link
Contributor

I tried this:

  • Copied ./exercies/Cargo.toml to my own clone of the rustlings repo (checked out to 4.0.0)
  • Opened ./examples in VS Code
  • Opened an exercise file

The Rust extension began working for quite a while, processing all the binaries. Eventually, it resulted in in-editor intellisense linting for each of the exercise files.

However, all of the problems from all files appear in the Problems panel at once. This might be all that is possible (I'm not sure how the Rust extension handles problem reporting). In comparison, the TypeScript language server only reports problems for files which are currently open in editor tabs.

Is there a way to get problem reporting without actually running all the debug incremental builds? It would speed things up tremendously.


Aside: Interestingly, this was among the reported problems:

~/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/src/libcore/macros/mod.rs
mod.rs[70, 17]: expected struct std::vec::Vec, found ()

mismatched types

expected struct `std::vec::Vec`, found `()`

note: expected struct `std::vec::Vec<std::string::String>`
      found unit type `()`rustc(E0308)

@jrvidal
Copy link
Contributor Author

jrvidal commented Jul 12, 2020

@jsejcksn Thanks for your testing. Out of curiosity, what analyzer were you using with VSCode? I don't get any errors about missing main() functions.

In fact, a plain old cargo check does not show errors for all files, which makes me think that this manifest will not work as intended 😞 It doesn't even seem deterministic 🙃

@jsejcksn
Copy link
Contributor

@jrvidal Whatever the Rust extension uses by default

@jrvidal
Copy link
Contributor Author

jrvidal commented Oct 26, 2020

I've been looking into this a bit more. The problem with an all-encompassing manifest is that cargo check chokes with so many targets, and bails out as soon as it finds 3 failing exercises or so.

I'll update the review soon trying a different approach: making the manifest point to the "current" exercise, per rustlings watch.

@jsejcksn
Copy link
Contributor

The problem with an all-encompassing manifest is that cargo check chokes with so many targets, and bails out as soon as it finds 3 failing exercises or so.

@jrvidal The strategy I shared in #443 (comment) works well to address that issue.

@jrmoulton
Copy link

Is there a reason that this was never merged?

@Zerotask
Copy link
Contributor

Zerotask commented May 8, 2021

@jrvidal I like your idea. Do you plan to update your PR?

@jrvidal
Copy link
Contributor Author

jrvidal commented May 8, 2021

It is an open question whether we want IDE integration or not: figuring out things "manually" might be a core part of the experience we want to provide. I wouldn't mind updating the PR, but I'd like to hear @fmoko's thoughts first.

@guyellis
Copy link

It is an open question whether we want IDE integration or not: figuring out things "manually" might be a core part of the experience we want to provide.

@jrvidal I think that the option to enable or disable IDE integration should up to the user. Each of us learns in a different way. For some they will have a better/faster learning experience not having IDE integration. Others will do better with it. I think that the best solution would be to provide the learner with the option to enable/disable and some docs on why they would want one or the other.

@manyinsects
Copy link
Member

Closing in favor of something like #911.

@manyinsects manyinsects closed this Feb 4, 2022
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.

6 participants