Skip to content
This repository has been archived by the owner on Sep 16, 2024. It is now read-only.

Implement change to only lowerCase the Windows drive letter instead of the entire file path #133

Merged
merged 4 commits into from
Apr 5, 2022

Conversation

hansenmc
Copy link
Contributor

@hansenmc hansenmc commented Jan 3, 2021

Resolves #123

@hansenmc
Copy link
Contributor Author

hansenmc commented Jan 3, 2021

Also, noted when running the tests on Windows that several of the tests in LoadModulesTest fail, due to verifyModuleCountWithPattern regex patterns using forward slashes, which are backslashes in Windows paths.

If you wanted them to work cross-platform, may need to adjust the pattern to look for either backslash or forard-slash: e.g. changing verifyModuleCountWithPattern(".*/ext/.*", "Should load every file", 7); to verifyModuleCountWithPattern(".*(/|\\\\)ext(/|\\\\).*", "Should load every file", 7);.

* @param file
* @return
*/
protected String normalizeDriveLetter(File file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay on this - I tried writing a test for this, but on Mac/Linux, "getRoot" always equals "/". Can you add tests for this - i.e. in a PropertiesModuleManagerTest (you'll want to rebase on dev) - that verify this on Windows? I assume the tests would just pass in Mac/Linux?

I was also wondering if an interface could be extracted here with "getPath()" and "getRoot()" methods. A real impl would be backed by a File. A test impl would be a mock that returns whatever we want. That allows us to test lines 157 through 164.

If you like the above idea, let me know, I'll toss it together and then merge this in.

@hansenmc hansenmc force-pushed the feature/130-caseSensitivePaths branch from 3255814 to d86bca9 Compare March 21, 2022 00:35
@ryanjdew ryanjdew self-requested a review April 5, 2022 19:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants