feat: module path aliases #2632
Conversation
corvusrabus
force-pushed
the
module_alias
branch
from
b364f32 to
4287e01
Compare
There was a problem hiding this comment.
See comments! I think that this can be made simpler by making the analysis function recursive. Also, I landed another PR which guarantees that a namepath is non-empty. Although the addition of new in this PR breaks that. Although I think the actual answer is using a recursive function and erroring when the path is empty. See comments.
src/analyzer.rs
Outdated
| })), | ||
| } | ||
| } | ||
|
|
||
| fn traverse_nonempty_module_name_path<'modules>( |
There was a problem hiding this comment.
I think this can be made simpler if it's recursive. Maybe with this signature:
fn foo(
path: &[Name],
modules: &Table<Justfile>,
recipes: &Table<Rc<Recipe>>,
) -> Result<Option<Rc<Recipe>>, ()>;
If the path is empty, then resolution has failed, and you can return Err(()).
If path is length one, then look in recipes for the target recipe.
Otherwise, try to find the next module to look in using the first item in path. If not found, then return an error. If found, then make the recursive call with path[1..], and the submodules recipes and modules.
There was a problem hiding this comment.
Hmm I didn't see added value in having an Err(()) and a Ok(None) so I made it just an Option with None indicating that the recipe was not found
src/analyzer.rs
Outdated
| .pop() | ||
| .expect("Internal error: Alias target path can't be empty"); | ||
|
|
||
| let alias = Alias { |
There was a problem hiding this comment.
I think that resolve can be changed so that it's implemented for the correct type, so we don't need to take apart and put the alias back together again.
|
Sorry I've been a bit busy. Will follow up on the weekend |
Co-authored-by: Casey Rodarmor <[email protected]>
corvusrabus
force-pushed
the
module_alias
branch
2 times, most recently
from
9de5027 to
31d6e83
Compare
corvusrabus
force-pushed
the
module_alias
branch
from
31d6e83 to
3dfc744
Compare
|
@casey I
Once we merge this I'd be happy to implement dependencies on recipes in other modules, which I think is 60% done with this MR already (that's the feature that I came here for in the first place :) ) |
src/namepath.rs
Outdated
| @@ -42,3 +63,11 @@ impl Serialize for Namepath<'_> { | |||
| serializer.serialize_str(&format!("{self}")) | |||
| } | |||
| } | |||
|
|
|||
| impl<'src> Deref for Namepath<'src> { | |||
There was a problem hiding this comment.
We shouldn't add deref to Namepath. It means that any function on Vec (I actually changed it to slice before I decided we shouldn't provide it at all.) is valid for namepath, which I think is not ideal. See comment about last later on.
src/analyzer.rs
Outdated
| mut modules: &'a Table<'src, Justfile<'src>>, | ||
| mut recipes: &'a Table<'src, Rc<Recipe<'src>>>, | ||
| ) -> Option<Rc<Recipe<'src>>> { | ||
| let name = path.last()?; |
There was a problem hiding this comment.
What happens if .last() return None? This will be treated as an unknown alias target, but is in fact a bug, since namepath should be non-empty, and thus if .last() returns None it should be an internal error. This is why we shouldn't iplement Deref for Namepath, because the function signature fn last(&self) -> Option<Name> doesn't make sense. It should be fn last(&self) -> Name.
There was a problem hiding this comment.
Will expect() panic now saying that Namepath can't be empty
src/namepath.rs
Outdated
| pub(crate) struct Namepath<'src>(Vec<Name<'src>>); | ||
|
|
||
| impl PartialEq for Namepath<'_> { |
There was a problem hiding this comment.
I don't think we should implement a PartialEq implementation which ignores everything but the lexeme.
I think we should do fn is_same_path(&self, other: &Self) -> bool which compares the lexemes.
src/namepath.rs
Outdated
| self.0.push(name); | ||
| } | ||
|
|
||
| fn lexeme_iter(&self) -> impl ExactSizeIterator<Item = &str> { |
There was a problem hiding this comment.
I don't think a helper function which is only used in one place adds much. .all(|(a, b)| a.lexeme() == b.lexeme()) is fine.
Co-authored-by: Casey Rodarmor <[email protected]>
| /// a::b::c -> (a::b, c) | ||
| /// a -> (empty, a) | ||
| pub fn split_last(&self) -> (&[Name<'src>], &Name<'src>) { | ||
| let name = self.last(); |
There was a problem hiding this comment.
Assigning this variable first instead of making this a one liner because last() will give a more meaningful panic message than &self.0[..self.0.len() - 1] panicking due to wrong indexing if len()==0
Implements #2629 .
The usage of
NamePathfeels clunky. Maybe I could make it so that it's guaranteed thatNamePathis not empty and that all tokens sit after each other separated by ColonColon. Then, we could generate a&strand avoid the usage ofStringinUnknownAliasTarget