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
base: master
from

Conversation

Projects
None yet
4 participants
@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

This comment has been minimized.

Show comment
Hide comment
@pastak

pastak Dec 3, 2015

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

pastak commented Dec 3, 2015

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

@oncletom

This comment has been minimized.

Show comment
Hide comment
@oncletom

oncletom Sep 22, 2016

Owner

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.

Owner

oncletom 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.

@oncletom oncletom closed this Sep 22, 2016

@bluejamesbond

This comment has been minimized.

Show comment
Hide comment
@bluejamesbond

bluejamesbond 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).

bluejamesbond 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).

@oncletom

This comment has been minimized.

Show comment
Hide comment
@oncletom

oncletom Nov 22, 2016

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?

crx/src/crx.js

Line 209 in d65dc24

ignore: ['*.pem', '.git', '*.crx']

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

Owner

oncletom commented Nov 22, 2016

@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?

crx/src/crx.js

Line 209 in d65dc24

ignore: ['*.pem', '.git', '*.crx']

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

@bluejamesbond

This comment has been minimized.

Show comment
Hide comment
@bluejamesbond

bluejamesbond 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.

bluejamesbond 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.

@oncletom

This comment has been minimized.

Show comment
Hide comment
@oncletom

oncletom Nov 24, 2016

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?

Owner

oncletom commented Nov 24, 2016

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