♻️ - Use package.path instead of manually constructing path part 2 #87
Conversation
| @@ -7,15 +7,17 @@ use regex::Regex; | |||
| use std::fs::File; | |||
| use std::io::prelude::*; | |||
|
|
|||
| use super::packages; | |||
There was a problem hiding this comment.
Is it ok? For logger to depend on packages doesn't look very nice, but here it actually makes sense.
| impl Package { | ||
| pub fn get_bs_build_path(&self) -> String { | ||
| format!("{}/lib/bs", self.path) | ||
| } | ||
|
|
||
| pub fn get_build_path(&self) -> String { | ||
| format!("{}/lib/ocaml", self.path) | ||
| } | ||
|
|
||
| pub fn get_mlmap_path(&self) -> String { | ||
| self.get_build_path() | ||
| + "/" | ||
| + &self | ||
| .namespace | ||
| .to_suffix() | ||
| .expect("namespace should be set for mlmap module") | ||
| + ".mlmap" | ||
| } |
There was a problem hiding this comment.
Or should I keep these functions in helpers.rs?
There was a problem hiding this comment.
No this is much nicer. I wanted to do this myself for some of the helpers that relate to structs. When I wrote some of the code I was still learning Rust :)
|
The code looks great. I also tested it on our monorepo, and I couldn't find any obvious issues. LGTM! |
|
@jfrolich What about merging. I'd like to continue working on pnpm support |
|
I think you can merge? |
|
No, only the maintainers of the repo can merge |
|
|
I would also appreciate if it could be merged and released, can't wait to try it on my setup! |
|
@tsnobip this is not a fix yet, but one step closer to it 😁 |
|
sorry for the confusion, we should make it possible to merge after approval. Merged now! |
|
@rolandpeelen let's make @DZakh a maintainer! |
It's a continuation of the change 1647b2a
I recreated the changes of the PR #84, because it was easier than solving merge conflicts. Also, I didn't include the change where I start resolving
pathdifferently to support pnpm, I want to do it in a separate PR, since this one is already quite big.