-
Notifications
You must be signed in to change notification settings - Fork 286
add dependency feature #304
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
Conversation
Signed-off-by: Luke-zhang-04 <[email protected]>
I've gotten started but I'm not sure if my code will even work. I haven't had the time to test any of it. My idea is that we can store all the package manager info in a hashmap or something similar (kind of like the |
@Luke-zhang-04, I pushed your implementation a little further; it now compiles. I think your design works well and can be extended to more package managers without difficulties. A thorough review will be done once the feature is usable (for npm at least). Looking forward to merge your PR 💯 |
Signed-off-by: Luke-zhang-04 <[email protected]>
To clarify, an |
From here
|
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
The |
Upstream was ahead of origin, and new .rustfmt.toml was added
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
…ter error handling
Well done @Luke-zhang-04 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR! This looks like it could be a pretty cool feature!
Left a few notes.
Signed-off-by: Luke-zhang-04 <[email protected]>
Is there a reason all the requested changes are marked as outdated? |
@spenserblack started his review before |
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
using map.contains_key is easier to understand and probably has better lookup time Signed-off-by: Luke-zhang-04 <[email protected]>
I think I fixed everything in the review 👍 |
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
The new things I added in CONTRIBUTING.md seem like they belong in the wiki. Or I can remove the examples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making all those changes! And thanks for updating the contributing guidelines!
I'm approving this overall, but I left a couple notes to catch edge-cases when the dependencies
key isn't defined.
Signed-off-by: Luke-zhang-04 <[email protected]>
Changes were made to CONTRIBUTING.md
Update branch for merging
I fixed the no dependency thing in 1d6d95b but I don't think I did it in the best way. I'm not sure though. |
Signed-off-by: Luke-zhang-04 <[email protected]>
Signed-off-by: Luke-zhang-04 <[email protected]>
I think that's really it. |
Thanks a lot for fixing the |
The go module parser was still broken, this seems to work: pub fn go_modules(contents: &str) -> Result<usize> {
let mut count = 0;
let mut start = false;
for line in contents.lines() {
if line.contains("require") {
start = true;
continue;
}
if start && line.contains(')') {
break;
}
if start {
count += 1;
}
}
Ok(count)
} |
Add dependency field