Skip to content

Pluggable project model #910

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

Merged
merged 1 commit into from
Jul 29, 2018
Merged

Pluggable project model #910

merged 1 commit into from
Jul 29, 2018

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Jul 27, 2018

This is a WIP at abstracting the project model, as proposed in #855 (comment).

I hope that it'll solve rust-lang/rls#956, but I haven't implemented the RLS side of things yet, so there's a chance that this whole approach breaks down.

Nevertheless, I figured I'd just toss it over the fence for now to see what the folks think :)

@matklad
Copy link
Contributor Author

matklad commented Jul 27, 2018

r? @kngwyu :)

@kngwyu
Copy link
Collaborator

kngwyu commented Jul 27, 2018

Great!
I feel we need to implement this kind of feature too, but now I don't have enough time, so really thank you for this PR.

But, I think it's better to merge this PR after implementing RacerProjectModelProvider to RLS.

@@ -0,0 +1,216 @@
use std::path::{Path, PathBuf};

pub trait RacerProjectModelProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need Racer prefix here?
From other crates it looks racer::RacerProjectModelProvider

if self.get_deps(manifest).is_none() {
// cache doesn't exist
// calculating depedencies can be bottleneck we use info! here(kngwyu)
info!("[get_outer_crates] cache doesn't exist");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please change function name here?

self.cache_deps(manifest, deps_map);
}
let deps = self.get_deps(manifest).unwrap();
debug!("[get_outer_crates] cache exists for manifest",);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above, please change function name in debug!.

let deps_map = self.resolve_dependencies(&manifest)?;
self.cache_deps(manifest, deps_map);
}
let deps = self.get_deps(manifest).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why call self.get_deps twice here

@kngwyu
Copy link
Collaborator

kngwyu commented Jul 27, 2018

I left some comments, but for the whole code, it looks good to me.

@matklad
Copy link
Contributor Author

matklad commented Jul 27, 2018

The RLS sideof things is rust-lang/rls#956. It's super ugly atm, but at least it works! Will look into clean-up of the racer implementation next! Thanks for the review.

This is mainly for RLS which would love to be a single source of truth
for project info, and which would rather link to a single copy of
Cargo.

However, it might also be possible to use other things as sources of
project information, for example, `cargo metadata` or Bazel/Buck build
rules.
@matklad
Copy link
Contributor Author

matklad commented Jul 27, 2018

Rebased and cleaned-up. The RLS side is super incomplete and ugly, but it works, and indeed get's rid of one Cargo :-)

@kngwyu
Copy link
Collaborator

kngwyu commented Jul 28, 2018

Now looks good enough to merge 👍
But I'm going to wait a little, in case you come up with an alternative idea while working on RLS side 🙂

@kngwyu kngwyu merged commit 18bd2c5 into racer-rust:master Jul 29, 2018
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