basicallydan / forkability

:fork_and_knife: A linter for your repository.
https://basicallydan.github.io/forkability
MIT License
103 stars 19 forks source link

Add .gitignore to lintFiles #39

Closed matiassingers closed 9 years ago

matiassingers commented 9 years ago

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:

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

M-Zuber commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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

M-Zuber commented 9 years ago

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

basicallydan commented 9 years ago

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 commented 9 years ago

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

basicallydan commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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