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

Check if the .gitignore is empty and if it is ignore node_modules. #422

Merged
merged 1 commit into from
May 29, 2019

Conversation

danielstaleiny
Copy link
Contributor

This pull request refer to #389

@zachleat
Copy link
Member

I like where you’re going here. I would prefer if we could get the file contents and trim to length so we could make this work if the file just has whitespace, too?

Also can you change this so it doesn’t modify any of the existing tests? Not sure why the folders needed to be changed there

@danielstaleiny
Copy link
Contributor Author

danielstaleiny commented Mar 11, 2019

Main reason for re-ordering the test was that I wanted them to be logically grouped.
if you have a look on the tests.

  • test("Get ignores (no .eleventyignore no .gitignore)"
  • test("Get ignores (no .eleventyignore .gitignore exists but empty)"
  • test("Get ignores (no .eleventyignore)"
  • test("Get ignores (no .eleventyignore, using setUseGitIgnore(false))"
  • test("Get ignores (no .gitignore)"
  • test("Get ignores (both .eleventyignore and .gitignore)"
  • test("Get ignores (both .eleventyignore and .gitignore, using setUseGitIgnore(false))"
  • test("Get ignores (both .eleventyignore and .gitignore, but .gitignore is empty)"

but if you insist on pushing the 2test and lest test at the end. I don't mind.

Added tests:
 - test no .eleventyignore, .gitignore exists but empty
 - test .eleventyignore, .gitignore exists but empty
 - .gitignore with spaces takes default
@danielstaleiny
Copy link
Contributor Author

I squashed the changes. all the changes are in the last commit.

@zachleat
Copy link
Member

Ah ah, I see. Yeah I can see the temptation for that. Modifying existing tests is a big red flag when I’m reviewing things—if you’d like to reorder them I’m happy to accept a new PR that does only this (thank you and sorry 😱)

@zachleat
Copy link
Member

I think this LGTM! Thank you so much!

@zachleat zachleat merged commit 7b749ee into 11ty:master May 29, 2019
@zachleat zachleat added this to the Next Minor Version milestone May 29, 2019
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