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

feat: do not update/commit files in .gitignore #230

Merged
merged 8 commits into from
Jan 3, 2018
Merged

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Dec 20, 2017

I've bumped into this issue a few times, and noticed a similar conversation in #213.

How does this solution look? @nexdrew, @satazor, @arobson?

@bcoe bcoe requested review from tomchentw and stevemao December 20, 2017 06:12
@satazor
Copy link

satazor commented Dec 21, 2017

Looks good, though tests are failing

Copy link
Member

@stevemao stevemao left a comment

Choose a reason for hiding this comment

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

LGTM!

Side note: Looks like lib/gitignore.js could be a separate npm package :)

lib/gitignore.js Outdated
@@ -0,0 +1,21 @@
const Dotignore = require('@bcoe/dotignore')
Copy link
Member

Choose a reason for hiding this comment

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

For Node 4 support, this file needs 'use strict' at the top. This fixes the failing tests for me locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nexdrew I guess I'll need to spin up a Windows VM ... grumble.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nexdrew smart 👍

@coveralls
Copy link

coveralls commented Dec 21, 2017

Coverage Status

Coverage decreased (-0.1%) to 99.548% when pulling dc21162 on respect-gitignore into 45fcad5 on master.

@nexdrew
Copy link
Member

nexdrew commented Dec 21, 2017

My attempt to fix the Windows build failed. Anyone else have ideas?

@bcoe
Copy link
Member Author

bcoe commented Dec 21, 2017

@nexdrew I noticed the underlying library has some logic that uses / rather than path.sep; I might take @stevemao's advice and write our own little library that uses find-up and will patch the issue.

@nexdrew
Copy link
Member

nexdrew commented Dec 21, 2017

@bcoe Are you talking about this line (and maybe line 9 as well)? I don't really think this would affect the failing test, but we could try setting matcher.delimiter = path.sep.

@nexdrew
Copy link
Member

nexdrew commented Dec 22, 2017

@bcoe I was able to spin up a Windows VM, and it looks like my suggestion of matcher.delimiter = path.sep actually works/fixes the failing test (which I find a bit surprising). Not sure my suggestion is the best long-term solution, but it might be enough to get this PR over the finish line.

@coveralls
Copy link

coveralls commented Dec 22, 2017

Coverage Status

Coverage decreased (-0.1%) to 99.548% when pulling 8f85f7e on respect-gitignore into 45fcad5 on master.

@coveralls
Copy link

coveralls commented Dec 22, 2017

Coverage Status

Coverage increased (+0.3%) to 100.0% when pulling 9efe36f on respect-gitignore into 45fcad5 on master.

Copy link
Member

@nexdrew nexdrew left a comment

Choose a reason for hiding this comment

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

@bcoe This should be ready to go, assuming you're still ok using the dotignore package.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 100.0% when pulling 9b8dc26 on respect-gitignore into 45fcad5 on master.

@coveralls
Copy link

coveralls commented Jan 3, 2018

Coverage Status

Coverage increased (+0.3%) to 100.0% when pulling 93d2624 on respect-gitignore into 45fcad5 on master.

@coveralls
Copy link

coveralls commented Jan 3, 2018

Coverage Status

Coverage increased (+0.3%) to 100.0% when pulling 2e9268b on respect-gitignore into 45fcad5 on master.

@bcoe bcoe merged commit 4fd3bc2 into master Jan 3, 2018
@bcoe bcoe deleted the respect-gitignore branch January 3, 2018 06:43
@Aigeec
Copy link

Aigeec commented Jan 23, 2018

@bcoe just wanted to let you know that I raised a bug on dotgitignore. It means that lines that are comments are included in the matches. So for example someone puts their directory name is as a comment the dotgitignore starts matching everything.

@chrisgalvan
Copy link

chrisgalvan commented Jul 7, 2018

@Aigeec Any update on when that will be fixed I just bump into that situation where my package.json and package-lock.json where ignored because some weird issue with https://github.com/bcoe/dotgitignore where it does match some ignored lines withe the full path of the package.json.

For example my package.json is at:
c:/user/myuser/dev/mypackage/package.json

and in my .gitignore I have the following

# ignore user files
.user*

because the path contains user package.json is being ignored.

@Aigeec
Copy link

Aigeec commented Jul 9, 2018

I've open a PR bcoe/dotgitignore#3 on https://github.com/bcoe/dotgitignore to resolve the issue but I believe @bcoe is triaging it at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants