Make modules with tests runnable #165

farodin91 · 2018-10-28T21:58:33Z

Fixes #154

I having problems to traverse the path to module. The main problem is that module_tree only supports FileId and not Module in files. Any idea?

I need to clean up the code a bit later.

matklad · 2018-10-30T17:50:13Z

The main problem is that module_tree only supports FileId and not Module in files.

Yeah, it indeed does not handle modules inside the file for the time being, so it's ok to just write custom code for modules in files. In the future, we need to modify module tree to work uniformly with all modules.

farodin91 · 2018-10-30T22:05:34Z

I would like to making the command line a bit better by adding the package. Any idea? To get crate name of a file.

matklad · 2018-10-30T22:15:31Z

I would like to making the command line a bit better by adding the package.

I think this line should add the neccessary --package foo --test bar arguments:
https://github.com/rust-analyzer/rust-analyzer/pull/165/files#diff-57a6a05c79db4a0461f95762e92206d3R314

farodin91 · 2018-10-30T22:35:16Z

If i'm correct line https://github.com/rust-analyzer/rust-analyzer/pull/165/files#diff-57a6a05c79db4a0461f95762e92206d3R310 already add the package.

matklad · 2018-10-31T07:34:57Z

@farodin91 yep, I've linked the wrong line

matklad · 2018-11-05T11:12:15Z

@farodin91 #193 adds inline modules to the module tree

aochagavia · 2018-11-10T14:46:00Z

Besides rebasing, what needs to happen so this PR can be merged?

farodin91 · 2018-11-10T15:16:53Z

The rebase is a bit complexer.

farodin91 · 2018-11-12T19:58:38Z

@matklad The module parent function seems to behave wrong. test_resolve_parent_module_for_inline_to_lib it returns the current mod.
I tried to resolve this problem, but i'm not sure what the problem is.

matklad · 2018-11-15T19:02:41Z

crates/ra_analysis/src/imp.rs

+
+        loop {
+            println!("current_position {:?}", current_position);
+            if let Some((file_id, file_symbol)) = self.parent_module(current_position)?.first() {


Hm, calling Analysis::parent_module is probably not the right thing to do here: this function is an API function used by LSP to implement "go to super module" feature.

We should add a path_to_root method to ModuleId instead, here:

https://github.com/rust-analyzer/rust-analyzer/blob/923483e321acace3bbf38688bd70d4d38f49b35e/crates/ra_analysis/src/descriptors/module/mod.rs#L82

then, we should use this method directly here, but constructing ModuleSource, converting it to ModuleId, etc, like we do in the implementation of Analysis::parent_module

this function is an API function used by LSP to implement "go to super module" feature.

This explains why, for inline modules, it returns the location of module's own identifier: this mirrors the way it woks for non-inline modules.

I try to get a the module name by the ModuleId and it seems a bit problematic.

Take a look at #234: it introduces a more OO-flavored API for modules, which hopefully should be easier to use.

In particular, a ModuleDescriptor now has a ::name method.

matklad · 2018-12-30T21:07:00Z

Oups, @farodin91 sorry for the long delay here: GitHub does not send notifications for force-pushes, so I've missed the latest developments. Will take a look soom!

DJMcNab

Looks good apart from a few nits and CI failing due to an invalid borrow

DJMcNab · 2018-12-30T21:07:54Z

crates/ra_analysis/src/imp.rs

@@ -184,6 +184,28 @@ impl AnalysisImpl {
        };
        Ok(query.search(&buf))
    }
+
+    pub(crate) fn module_path(&self, position: FilePosition) -> Cancelable<String> {


I feel like this should return Cancelable<Option<String>> as opposed to returning Ok("") in the failure cases.

DJMcNab · 2018-12-30T21:08:31Z

crates/ra_analysis/src/imp.rs

+        let path = modules
+            .into_iter()
+            .filter_map(|s| s.name())
+            .skip(1)


I'm sure it's there for a reason, but a comment explaining what skip(1) does in this case would be useful.

matklad · 2018-12-30T21:14:57Z

crates/ra_analysis/src/imp.rs

+        let modules = descr.path_to_root();
+
+        let path = modules
+            .into_iter()


Changing this .into_iter() into .iter() should fix the borrowing error.

DJMcNab · 2018-12-31T11:12:44Z

crates/ra_analysis/src/imp.rs

+        let path = modules
+            .iter()
+            .filter_map(|s| s.name())
+            .skip(1) // name is already part of the string.


Thanks!

EDIT: Maybe we can clarify that it's used as the fold root.

DJMcNab · 2018-12-31T12:22:14Z

bors delegate+

This looks good - all that's missing is you have an extra import.

bors · 2018-12-31T12:22:15Z

✌️ farodin91 can now approve this pull request

Fixes rust-lang#154

farodin91 · 2018-12-31T14:15:20Z

bors r+

165: Make modules with tests runnable r=farodin91 a=farodin91 Fixes #154 I having problems to traverse the path to module. The main problem is that module_tree only supports `FileId` and not `Module` in files. Any idea? I need to clean up the code a bit later. Co-authored-by: Jan Jansen <[email protected]>

bors · 2018-12-31T14:20:18Z

Build succeeded

And happy new year from bors! 🎉

continuous-integration/travis-ci/push

farodin91 force-pushed the modules-with-tests-runnable branch from 3fa32cc to 09b7adc Compare October 30, 2018 22:04

farodin91 force-pushed the modules-with-tests-runnable branch from 09b7adc to cdde466 Compare November 12, 2018 19:57

matklad reviewed Nov 15, 2018

View reviewed changes

farodin91 force-pushed the modules-with-tests-runnable branch from cdde466 to da79637 Compare November 26, 2018 22:20

farodin91 force-pushed the modules-with-tests-runnable branch from da79637 to 13b7985 Compare December 27, 2018 20:45

DJMcNab approved these changes Dec 30, 2018

View reviewed changes

matklad reviewed Dec 30, 2018

View reviewed changes

farodin91 force-pushed the modules-with-tests-runnable branch from 13b7985 to 9e5a896 Compare December 31, 2018 11:10

DJMcNab reviewed Dec 31, 2018

View reviewed changes

farodin91 force-pushed the modules-with-tests-runnable branch from 9e5a896 to 1a46eff Compare December 31, 2018 11:26

Make modules with tests runnable

05daa86

Fixes rust-lang#154

farodin91 force-pushed the modules-with-tests-runnable branch from 1a46eff to 05daa86 Compare December 31, 2018 14:00

bors bot merged commit 05daa86 into rust-lang:master Dec 31, 2018

sify21 mentioned this pull request Sep 16, 2020

ra_lsp_server keeps hogging one core #2812

Closed

Make modules with tests runnable #165

Make modules with tests runnable #165

Uh oh!

Conversation

farodin91 commented Oct 28, 2018 • edited by aochagavia Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Oct 30, 2018

Uh oh!

farodin91 commented Oct 30, 2018

Uh oh!

matklad commented Oct 30, 2018

Uh oh!

farodin91 commented Oct 30, 2018

Uh oh!

matklad commented Oct 31, 2018

Uh oh!

matklad commented Nov 5, 2018

Uh oh!

aochagavia commented Nov 10, 2018

Uh oh!

farodin91 commented Nov 10, 2018

Uh oh!

farodin91 commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad Nov 15, 2018

Choose a reason for hiding this comment

Uh oh!

matklad Nov 15, 2018

Choose a reason for hiding this comment

Uh oh!

farodin91 Nov 18, 2018

Choose a reason for hiding this comment

Uh oh!

matklad Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

matklad commented Dec 30, 2018

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

DJMcNab Dec 30, 2018

Choose a reason for hiding this comment

Uh oh!

DJMcNab Dec 30, 2018

Choose a reason for hiding this comment

Uh oh!

matklad Dec 30, 2018

Choose a reason for hiding this comment

Uh oh!

DJMcNab Dec 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab commented Dec 31, 2018

Uh oh!

bors bot commented Dec 31, 2018

Uh oh!

farodin91 commented Dec 31, 2018

Uh oh!

bors bot commented Dec 31, 2018

Build succeeded

Uh oh!

Uh oh!

farodin91 commented Oct 28, 2018 •

edited by aochagavia

Loading

farodin91 commented Nov 12, 2018 •

edited

Loading

DJMcNab Dec 31, 2018 •

edited

Loading