Skip to content
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: Add CMakeCache.txt detection to CMake module #1795

Merged
merged 5 commits into from
Oct 25, 2020

Conversation

cschlosser
Copy link
Contributor

@cschlosser cschlosser commented Oct 15, 2020

Description

For CMake it also makes sense to check if we're currently in the build folder.
This is done by checking for the existence of CMakeCache.txt.

Motivation and Context

Now it also marks a build folder as CMake related.
This is actually where must users will spend most of their time as it's the location where the actual build system is called.

Screenshots (if appropriate):

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@cschlosser cschlosser changed the title Add CMakeCache.txt detection to cmake module feat: Add CMakeCache.txt detection to CMake module Oct 15, 2020
Copy link
Member

@andytom andytom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few suggestions to fix the changes can you also update the config docs to match these changes.

@@ -77,7 +77,17 @@ mod tests {
let dir = tempfile::tempdir()?;
File::create(dir.path().join("CMakeLists.txt"))?.sync_all()?;
let actual = ModuleRenderer::new("cmake").path(dir.path()).collect();
let expected = Some(format!("via {} ", Color::Blue.bold().paint("喝 v3.17.3")));
let expected = Some(format!("via {} ", Color::Blue.bold().paint("喝 v3.17.3")));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what has happened here but it looks like the symbol used here has changed and that is why the tests are failing.

Suggested change
let expected = Some(format!("via {} ", Color::Blue.bold().paint(" v3.17.3")));
let expected = Some(format!("via {} ", Color::Blue.bold().paint(" v3.17.3")));

let dir = tempfile::tempdir()?;
File::create(dir.path().join("CMakeCache.txt"))?.sync_all()?;
let actual = ModuleRenderer::new("cmake").path(dir.path()).collect();
let expected = Some(format!("via {} ", Color::Blue.bold().paint("喝 v3.17.3")));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

Suggested change
let expected = Some(format!("via {} ", Color::Blue.bold().paint(" v3.17.3")));
let expected = Some(format!("via {} ", Color::Blue.bold().paint(" v3.17.3")));

@cschlosser
Copy link
Contributor Author

Thanks for taking a look at it and figuring this out.
Even Github seems to have problems with it:
Suggestion cannot be identical to original text. Please refresh and try again.
Seems like I have to do this locally.

I will update the docs once the tests are working.

@andytom andytom merged commit 89fc93d into starship:master Oct 25, 2020
@andytom
Copy link
Member

andytom commented Oct 25, 2020

Thanks for your contribution @cschlosser

chipbuster pushed a commit to chipbuster/starship that referenced this pull request Jan 14, 2021
)

* Add CMakeCache.txt to cmake module

* Remove trailing whitespace

* Apply fixes by @andytom

* Add CMakeCache.txt to docs

* Revert documentation for languages other than en
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants