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

Implement .crxignore #51

Closed
wants to merge 3 commits into from
Closed

Implement .crxignore #51

wants to merge 3 commits into from

Conversation

augmt
Copy link

@augmt augmt commented Dec 3, 2015

This pull request builds upon #49: .crxignore is read asynchronously; there is an appropriate CLI command; there is a new test in place; and the documentation has been updated.

If it's preferable I can submit the pull request to pastak/crx so that @pastak may then submit their pull request.

@pastak
Copy link

pastak commented Dec 3, 2015

@augmt Good work 👍 ( I have not worked it for busy sorry... 🙇 )

@thom4parisot
Copy link
Owner

thom4parisot commented Sep 22, 2016

Hey guys,

although I feel ignore files or selecting them in a finer way (cf. #66) would help, I don't believe supporting an ignore file is the first step to implement it.

I'd rather have a good and solid API then add an ignore file if this is a common need. I'd rather stay close to the CLI rather than having an implicit configuration file – we already have too many of them.

I hope you don't mind me closing your proposal.

@mathew-kurian
Copy link

mathew-kurian commented Nov 22, 2016

This a very useful features especially when your building on transpiled environment, so I have kept a mirror of this commit if anyone wants to install it via. npm: https://github.com/bluejamesbond/crx (credit provided @pastak @augmt).

@thom4parisot
Copy link
Owner

@bluejamesbond you are right – could you eventually just implement something which helps configure the following ignore list, from the API and CLI point of view?

https://github.com/oncletom/crx/blob/d65dc24d340078e48f918b2ebff44b580a40291a/src/crx.js#L209

Do you think it would it provide the same level of feature as the ignore file?

@mathew-kurian
Copy link

mathew-kurian commented Nov 23, 2016

In my opinion, a crxignore file is just a simpler. It provides a way to easily maintain the codebase. If a new folder is added, the workflow is quite easy. I just update all my ignore files. In the event we opt for a CLI, then either I have an extra script file which contains the command with all the ignore files appended or I have it in package.json. Updating a script and/or the package.json isn't as easy.

The crxignore also allows for nested ignores i.e. if I have a module shared across multiple extensions.

And of course, this isn't to say a API/CLI option is not need but that it should be coupled with the crxignore.

@thom4parisot
Copy link
Owner

Oh interesting use case! What is the layout of your repos/code then?

To me, having a configurable exclusion/files whitelisting at the API level is more important because then it is just a matter of how you feed them (via CLI, via ignore file, via env variables etc.) so I suggest to do it in 3 moves:

  1. excluding at API level
  2. excluding at CLI level
  3. excluding at ignore file level

Although we have to be careful because the gitignore spec works differently than the minimatch one.

@bluejamesbond because the branch diverged, would you be happy to (re)submit any of the above points as a 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.

None yet

4 participants