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 issue #498 - c8 CLI doesn't work properly when executed in a sub-directory of the project #539

Closed
wants to merge 1 commit into from

Conversation

ericmorand
Copy link

@ericmorand ericmorand commented Jun 23, 2024

This PR fixes the issue #498...and also change the philosophy of c8.

The fix

This PR makes c8 consider the include and exclude patterns as relative to the configuration file location, if any, or the current working directory, otherwise. By applying such a change, c8 can be executed from any subdirectory of the project and will always resolve sources to include and exclude regardless of the current working directory, if a configuration file is present.

The philosophy shift

c8 philosophy seems to have been heavily inspired by nyc, which means that it inherits from nyc debatable (and arguably archaic) design decisions. These decisions can be summarized this way:

Options src, include, exclude, extension, excludeNodeModules, allowExternal and all are overlapping and can be reduced to only include and exclude without removing any feature.

This PR removes all the mentioned options except include and exclude, and updates the logic so that all the removed options are organically available through the include and exclude options.

Let's see how it goes.

--src

This option is overlapping with the include option, which is the declaration of the files that we want to be covered by the tests:

--include=src/main/**/*.js

--extension

This option is overlapping with the include option, which is the declaration of the files that we want to be covered by the tests:

--include=src/main/**/*.ts --include=src/main/**/*.tsx

Or:

--include=src/main/**/*.{ts,tsx}

--allowExternal

This option is overlapping with the include option, which is the declaration of the files that we want to be covered by the tests:

--include=src/main/**/*.ts --include=../../../../external-project/*.js

--excludeNodeModules

This option is overlapping with the exclude option, which is the declaration of the files that we don't want to be covered by the tests:

--include=**/*.js --exclude=node_modules/**/*

--all

This option is overlapping with the include and exclude options, would we want to not include all of them, it is possible through a combination of include and exclude:

--include=**/*.js --exclude=**/not-included/*.js

This is obviously a breaking change.

@@ -499,100 +496,6 @@ beforeEach(function () {
output.toString('utf8').should.matchSnapshot()
})
})
describe('--all', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm concerned about removing so many tests in this PR.

@bcoe
Copy link
Owner

bcoe commented Jun 26, 2024

@ericmorand I understand where you're coming from, but this breaking change would be pretty drastic, and I don't have the time to put into open source these days to support it.

Could we instead figure out how to make --allowExternal work for your case, and fix any bugs you bump into.

@ericmorand
Copy link
Author

@bcoe , your point is fair and I actually agree that this is too much of a drastic change, even for a breaking change: this is like a different product. I'll probably end up proposing a dedicated package - heavily inspired by your great work. I think we can close this PR.

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