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

Add .gitignore to lintFiles #39

Merged
merged 3 commits into from
Dec 20, 2014
Merged

Add .gitignore to lintFiles #39

merged 3 commits into from
Dec 20, 2014

Conversation

matiassingers
Copy link
Contributor

First step is to check for a .gitignore file in the repo.


As @basicallydan suggested in PR #38, perhaps a linter for common error files like:

  • .DS_Store
  • .Trashes
  • .Spotlight-V100
  • .idea
  • *.sublime-*

What do you think of adding a lintIgnore or something like that?

@M-Zuber
Copy link
Collaborator

M-Zuber commented Dec 11, 2014

What kind of files show up in a general .gitignore though?
Methinks that in most case they are language specific, and therefore should be defined in the corrosponding language file

@matiassingers
Copy link
Contributor Author

@M-Zuber agreed, a lot of the files in a .gitignore are definitely language specific - but a general linter for .gitignore might make that process easier(kind of like files now).

It's also weird to suggest that some files are required in a .gitignore, like .DS_Store should be in a persons global list instead of the local .gitignore.

@M-Zuber
Copy link
Collaborator

M-Zuber commented Dec 11, 2014

I am sorry, but I didn't understand your second point.
How are you thinking the module would work?
What is coming to my mind is adding an object to each language object so that they look like this:

{
   name: 'language name',
   files: {
   ...
   },
   gitignore: {
   ...
   }
}

and then in the linter it would perform the same task as the file linter (for example)?

@matiassingers
Copy link
Contributor Author

@M-Zuber my explanation was quite poor, but yes exactly like you explained!

@M-Zuber
Copy link
Collaborator

M-Zuber commented Dec 11, 2014

I've created an issue for a cleaner discussion. #41
Lets see what @basicallydan has to say about it

@basicallydan
Copy link
Owner

NOTE: Comment is relevant to #41, moved there.

@M-Zuber wrote
What is coming to my mind is adding an object to each language object so that they look like this:
example - see #41

Yes, just like that!

@matiassingers wrote
It's also weird to suggest that some files are required in a .gitignore, like .DS_Store should be in a persons global list instead of the local .gitignore.

Good point. However, as @jonfinerty pointed out offline:

The .gitignore isn't just for the project owner, it's to prevent people who clone it from accidentally uploading ignored files.

So e.g., you have .DS_Store ignored globally, so you don't put it in your .gitignore, and then someone without it ignored globally clones your repo, makes a fork and tries to commit .DS_Store because the .gitignore doesn't exclude it. See what I mean?

So to summarise, I think a global list of files to be expected in .gitignore makes sense, as well as some language-specific ones which are linted in a similar way to how we currently lint language-specific files, e.g. in NodeJS.

In the end, the role of the lintFiles step is there to ensure that files we don't want don't exist, and that required files do exist, and the lintIgnore step is there to ensure that new collaborators don't accidentally commit files that should be ignored, because .gitignore will prevent that.

Great discussion, guys! Loving this progress, thanks for helping out :)

@matiassingers
Copy link
Contributor Author

@basicallydan you are completely right! Would be awesome to add these suggestions, guess we can track it in #41 from here on out.

@basicallydan
Copy link
Owner

@matiassingers Cool, yes! I'll move my comment over there.

In the meantime, can you do one small thing for this PR before I merge please? Just add a test or two to make sure that the .gitignore file lint works (it can be caps or no caps, so two tests should do it)? I believe the file actually needs to be called .gitignore, too, rather than gitignore without the dot. I might be wrong about that but I just tried getting rid of the dot and it didn't work. Thoughts?

@matiassingers
Copy link
Contributor Author

@basicallydan yes of course, totally forgot about the test spec.

Tried to keep the current format of the test spec, let me know if anything else needs fixing.
Do you want me to squash all the commits?

@basicallydan
Copy link
Owner

@matiassingers No, that's fine :) Looks great. I'll merge and version later on. Thanks!

@basicallydan basicallydan merged commit 128cce1 into basicallydan:master Dec 20, 2014
@matiassingers matiassingers deleted the patch-1 branch December 20, 2014 21:54
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.

3 participants