-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat(nim): add support for nimble project package version #2569
Conversation
src/modules/package.rs
Outdated
@@ -175,6 +186,8 @@ fn extract_meson_version(file_contents: &str) -> Option<String> { | |||
fn get_package_version(base_dir: &Path, config: &PackageConfig) -> Option<String> { | |||
if let Ok(cargo_toml) = utils::read_file(base_dir.join("Cargo.toml")) { | |||
extract_cargo_version(&cargo_toml) | |||
} else if utils::find_file(base_dir, |file_name| file_name.ends_with(".nimble")).is_some() { |
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.
Can you have this at least make the initial check for nimble
files via context
, like the language modules do? It keeps a list of files and file extensions.
context
.try_begin_scan()?
.set_extensions(&["nimble"])
.is_match();
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.
Thank you for the suggestion. It's done.
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.
I think the current approach and not always calling nimble dump
is better for general performance.
It has been complete from my side now. I don't own a dev macos or windows machine. |
project_dir.close() | ||
} | ||
|
||
#[test] |
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.
Another test with a nimble file but no nimble command available would be nice.
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.
Sure. I've just added it.
That's fine. |
Let me try something before we merge this, looks good otherwise 👍 |
Ignore me, my attempts failed so let's continue reviewing it as is. |
Should I rebase and resolve conflicts? |
Please do 🚀 |
885b5e3
to
f189d6c
Compare
Done. |
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.
LGTM
Sorry @aeruhxi can you fix the merge conflicts again and then we can get this merged. |
Sure, no problem. |
Co-authored-by: David Knaack <davidkna@users.noreply.github.com>
I don't know why it did not come up while I was fixing merge conflicts on rebase.
f189d6c
to
65286ce
Compare
This PR adds support for the package module for a nimble project to show version number in the project directory.
Description
I have added a new functionfind_file
that takes a directory and a predicate. It iterates over all files in a given directory and returns a filename if it satisfies the given predicate. This function is used to detect if a given directory is a nimble project by checkingfile_name.ends_with(".nimble")
.If a directory is found to be a nimble project,
extract_nimble_version
then callsnimble dump --json
to get the package version. I usedjson
instead of the defaultini
format for dump because I could not find an existingini
parser used in the project butjson
parser was already there used predominantly for most packages.Update
It now uses context to check for the existence of
.nimble
file.Alternative implementation?
After already writing
find_file
, I later found out that Instead of usingfind_file
, I could directly callnimble dump
and checks its exit code, because it fails if it's not a nimble project, to determine a nimble project. I want to have others' opinion on this if that would be a better implementation idea.Motivation and Context
The motivation and context is in the linked issue.
Closes #2454.
Screenshots (if appropriate):
How Has This Been Tested?
Checklist:
This PR is a WIP. Docs and tests are yet to be done.