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

Fix file/directory name case #650

Merged

Conversation

JaredReisinger
Copy link
Contributor

The code references don't always match the actual filenames in case. For example, ./src/index.js refers to ./World/World, but the actual path is ./world/World(.js). This may work on macOS and Windows (which are case-insensitive by default), but not on Linux or on other case-sensitive systems.

This PR renames the files/directories based on what appears to be their canonical names (based on the source references and other directories). Only two files (scripts/pack.js and src/Doc/methods/output/_debug.js) needed to be updated/edited to work with the new names.

Running npm test now succeeds 100% with no error messages on my system (WSL/ext4 on Windows), once I'd npm install'd in all of the plugin directories.

Fixes #649

The code references don't always match the actual filenames in case.  For example, `./src/index.js` refers to `./World/World`, but the actual path is `./world/World(.js)`.  This may work on macOS and Windows (which are case-insensitive by default), but not on Linux or on other case-sensitive systems.

This PR renames the files/directories based on what appears to be their canonical names (based on the source references and other directories).  Only two files (`scripts/pack.js` and `src/Doc/methods/output/_debug.js`) needed to be updated/edited to work with the new names.

Running `npm test` now succeeds 100% with no error messages on my system (WSL/ext4 on Windows), once I'd `npm install`'d in all of the plugin directories.

Fixes spencermountain#649
@JaredReisinger
Copy link
Contributor Author

FWIW, npm run testb, and npm run testOne also pass 100%. Sadly npm run test:spec fails, but that's because tap-spec isn't listed as a devDependency. Hand-running that as npx tape "./tests/**/*.test.js" | npx tap-spec does succeed 100%.

JaredReisinger added a commit to JaredReisinger/compromise that referenced this pull request Dec 17, 2019
… handling a greedy match.

When handling a greedy match that's anchored to the start (`^`) or end (`$`), it is acceptable (normal, in fact!) for some of the matched terms to **not** be the first or last term in the phrase.  This fix attempts to allow that without accidentally forgetting that the greedy match as a whole should still either start or end the document.

Fixing this caused the `Doc.verbs()` behavior at https://github.com/spencermountain/compromise/blob/master/src/Subset/verbs/index.js#L15-L16 to change slightly (and correctly, I'd argue), such that a small change was needed at https://github.com/spencermountain/compromise/blob/master/src/Subset/verbs/methods.js#L45 (and on line 50) to compensate.

Added a test to specifically check for the anchored-greedy-match case.

(Note that this was fixed assuming PR spencermountain#650, so that I could run the tests.)

Fixes spencermountain#648
@spencermountain
Copy link
Owner

ahh, thank you Jared. Just got back from vacation, will look at this this afternoon.
❤️

@spencermountain
Copy link
Owner

thank you!

@spencermountain spencermountain merged commit 59bd1dd into spencermountain:master Dec 18, 2019
Drache93 pushed a commit to Drache93/compromise that referenced this pull request Jan 6, 2020
… handling a greedy match.

When handling a greedy match that's anchored to the start (`^`) or end (`$`), it is acceptable (normal, in fact!) for some of the matched terms to **not** be the first or last term in the phrase.  This fix attempts to allow that without accidentally forgetting that the greedy match as a whole should still either start or end the document.

Fixing this caused the `Doc.verbs()` behavior at https://github.com/spencermountain/compromise/blob/master/src/Subset/verbs/index.js#L15-L16 to change slightly (and correctly, I'd argue), such that a small change was needed at https://github.com/spencermountain/compromise/blob/master/src/Subset/verbs/methods.js#L45 (and on line 50) to compensate.

Added a test to specifically check for the anchored-greedy-match case.

(Note that this was fixed assuming PR spencermountain#650, so that I could run the tests.)

Fixes spencermountain#648
@JaredReisinger JaredReisinger deleted the fix-filename-case branch August 7, 2020 18:22
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.

Can't easily run tests on case-sensitive file-system
2 participants