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

Added support for .xpiignore (at sourceDir) with node-ignore #703

Closed
wants to merge 1 commit into from

Conversation

mathew-kurian
Copy link

@mathew-kurian mathew-kurian commented Dec 22, 2016

This pull request is just to see if you are interested. I can refractor and add tests as needed.

EDIT:
Another module worth considering you think this feature can be adopted - fsstream-ignore

Fixes #131

@coveralls
Copy link

coveralls commented Dec 22, 2016

Coverage Status

Coverage decreased (-0.8%) to 99.228% when pulling b6fc9dc on bluejamesbond:master into dbdf9e2 on mozilla:master.

@kumar303
Copy link
Contributor

Hi @bluejamesbond , thanks for getting in touch about this!

I would love to help you land this patch but I think it would be better to approach it like this:

  • implement it as a command line option, such as --ignore-files=*.zip,*.xpi
  • stick with minimatch unless there is a compelling reason to support the exact gitignore matching?

The reason I'm suggesting a command line option instead of a .web-ext-ignore file is because we will soon have config parsing support in #176 . After that, the option could be set in a web-ext-config.js file like:

module.exports = {
  ignoreFiles: [
    '*.zip',
    '*.xpi',
  ]
};

Regardless, we definitely need a command line option first because currently the ignore list isn't configurable from the CLI.

@kumar303
Copy link
Contributor

Your patch would definitely address #131, thanks for getting it started!

@Croydon
Copy link

Croydon commented Jan 17, 2017

@kumar303 Is the web-ext-config.js file you have in mind a local project file or global? I'm looking for a local .webextignore like .jpmignore

@kumar303
Copy link
Contributor

The web-ext-config.js could be local (per project) or global in ~/.web-ext-config.js. Both would be supported.

]);

if (sourceDir) {
const ignoreFilePath = path.join(sourceDir, '.xpiignore');
Copy link
Member

Choose a reason for hiding this comment

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

Don't want to torpedo this PR, but something like .webextignore seems more appropriate to me.

Copy link
Author

Choose a reason for hiding this comment

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

I had little free time last month so I couldn't follow up on this PR. It seems the project is opting for a *.js file and not using an ignore file I believe. But yes, a .webextignore is a more apt name.

Copy link

Choose a reason for hiding this comment

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

Agreed. In special since .xpi ist not anymore the only supported file ending.

@kumar303 kumar303 self-assigned this Jan 17, 2017
@kumar303
Copy link
Contributor

By the way, I just remembered that we have a newer active patch for this #753 (perhaps the contributor will be able to land it). Thanks again for helping to kick this off though.

@mathew-kurian
Copy link
Author

mathew-kurian commented Jan 18, 2017

@kumar303 no problem. My apologies for not catching up. Something other things had come up.

@kumar303
Copy link
Contributor

No worries at all! Thanks for your interest in web-ext. I'll close this one since #753 is pretty close to landing.

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.

None yet

6 participants