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

Linter for files in gitignore #41

Open
M-Zuber opened this issue Dec 11, 2014 · 21 comments
Open

Linter for files in gitignore #41

M-Zuber opened this issue Dec 11, 2014 · 21 comments

Comments

@M-Zuber
Copy link
Collaborator

M-Zuber commented Dec 11, 2014

This was first raised in #39
The idea is to add a gitignore field to the language object that will contain the files that are supposed to be not pushed to the repo due the fact that are in the .gitignore file

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

Then there would be a separate linter to perform the checks

@basicallydan
Copy link
Owner

My comment on this from #39

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

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Dec 11, 2014

Here is a list of .gitignore templates

@basicallydan
Copy link
Owner

@M-Zuber Thanks! Quite a lot to get through!

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Dec 11, 2014

It can also serve as a checklist of sorts for languages to support 😉

@basicallydan
Copy link
Owner

Hahah, true yeah!

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Jul 29, 2015

I merged #51.

Is that what we want and this is done, or do we want to upgrade it to download and parse the .gitignore in the project?

Thoughts on that:

  • Part of the defaults is making sure that the .gitignore exists in the first place.
  • Is there such a thing as having multiple .gitignore files in one repo?

@basicallydan
Copy link
Owner

Ooh. That would be good, but would it break the web interface? Argh. Hmm. The web interface is pretty cool, I love the fact that it works in the browser or on the command line. Does GitHub possibly give access to .gitignore files through CORS JavaScript requests?

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Jul 29, 2015

I'll look into the api, and test it out. Definitely agree that we can't break the web

@basicallydan
Copy link
Owner

Cool! Let me know when you get to it, if I get some time sooner then I'll do it :)

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Jul 30, 2015

CORS resource sharing
Downloading a file
These look like the main resources that need to be investigated. In addition I sent an email to a GitHubber I've interacted with in the past to get feedback on the concept of downloading the .gitignore

I am starting to think that it does not make sense. What I might do is try and create a tool that will

  • for each language we currently support
    • get .gitignore template using this api
    • somehow convert the values into key/value + regex the value in the language files

Or do it manually ;)
What say you?

@basicallydan
Copy link
Owner

I like the idea of using the GitHub API to get the gitignore templates because it helps us to suggest what people probably shouldn't have in the repo. But this may change quite a bit for every project, so it may not be such a popular idea.

Worth a try, though, I think. We can always have a "don't use gitignore" flag. We'd have to use pretty much the exact same algorithm as git to make this work though, right?

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Aug 2, 2015

Your first point is something that I have been trying to come up with a good solution for.
One possibility is (in the not so near future) to update the web interface - not sure how to deal with the CLI - to have a picker for the ruleset, where the user can choose exactly what they want.
Because, the same way not every project follows the .gitignore template exactly - they may also not want to be held to some of the other rules we have.
TL;DR All our rules are subjective, and this particular check is the most invasive - so making it optional is probably the best way to go.

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Aug 2, 2015

As far as the algorithm goes, I am not sure.
Is the algorithm used by git available?

@basicallydan
Copy link
Owner

A quick Google search doesn't bring anything up, but I don't think it'd be too hard to come up with a simple version. Most rules would be a matter of checking for existence using node's native filesystem stuff, and in some instances we'd wanna be recursive.

As for making things optional, index.js already has some code in there for CLI stuff, so perhaps just adding a new flag for each linter would work, or a flag which accepts comma separated linter names. Then that gets passed through to the options of the main code called by both the web ui and CLI.

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Aug 5, 2015

I've been speaking to someone from github who works with the .gitignore template repo.
He gave a sketch for a flow to use where we to go with the option of using the users .gitignore, still waiting for feedback on just using the defaults.

In the meantime, I think I want to start with taking the information from the defaults.
I'll write a standalone tool - probably only going to be useful for this repo.
A few q's on that;

  • Should i put it under my name, or do we want to create an org?
  • Does it matter what tech its in?
  • Should it update the existing language files, or create new files in a format that the linter recognizes?

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Aug 5, 2015

As for making things optional, index.js already has some code in there for CLI stuff, so perhaps just adding a new flag for each linter would work, or a flag which accepts comma separated linter names. Then that gets passed through to the options of the main code called by both the web ui and CLI.

So I don't really know that part of the codebase to well yet. But it sounds like its easy enough to go either way, that we can leave this decision until everything else is implemented?

@basicallydan
Copy link
Owner

Should i put it under my name, or do we want to create an org?

Let's just stick with using name repos for now, don't want too much bureaucracy just yet.

Does it matter what tech its in?

Well presumably it'd have to be JS unless you wanna put some binaries of other things into the repo, right?

Should it update the existing language files, or create new files in a format that the linter recognizes?

I figured that however the gitignore defaults thing works it'd be used as a module which simply retrieves gitignore defaults and then works in a similar way to the lintFiles linter. Does that make sense, or did I misunderstand your question?

So I don't really know that part of the codebase to well yet. But it sounds like its easy enough to go either way, that we can leave this decision until everything else is implemented?

Yeah, that's not a problem :)

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Aug 6, 2015

Does it matter what tech its in?

Well presumably it'd have to be JS unless you wanna put some binaries of other things into the repo, right?

I actually was thinking of having a tool that is run every now and then, not in relation to forkability's actual code in order to do set up

For example a c# .exe that can be set to run once a day/week that will update given files in the forkabilty repo. This way, during runtime no additional web calls have to be made
What do you think?

Should it update the existing language files, or create new files in a format that the linter recognizes?

I figured that however the gitignore defaults thing works it'd be used as a module which simply retrieves gitignore defaults and then works in a similar way to the lintFiles linter. Does that make sense, or did I misunderstand your question?

That is pretty much what happens now:

for (var i = options.languages.length - 1; i >= 0; i--) {
  if (languages[options.languages[i]]) {
    lintOptions = merge(lintOptions, languages[options.languages[i]]);
    } else {
        console.warn('Sorry, Forkability doesn\'t support the language', options.languages[i] + '. If you\'re keen you can open a pull request at https://github.com/basicallydan/forkability/issues');
    }
}
...
if(options.ignore || lintOptions.ignore) {
    var ignoreOptions = merge(options.ignore, lintOptions.ignore);

    report = lintIgnore(tree, ignoreOptions);
    passes = passes.concat(report.passes);
    failures = failures.concat(report.failures);
}

and in a language file:

module.exports = {
    name: 'Node JS',
    files: {
        'package.json file':/^package\.json/i,
        'No node_modules folder': {
            path: /^node_modules/i,
            shouldExist: false,
            type:'tree'
        }
    },
    ignore:{
         'randomExtension': /^.+\.rnd/i
     }
};

At the same time, the ignore key could be stored elsewhere, maybe a set of files matching the structure of the language files?

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Dec 27, 2015

I have thought about this a bunch more and have come to the conclusion that gitignore linting can only be based off the gitignore currently in the repository.
Makes this a much simpler change of simply adding a step that reads the file if it exists and then parses each line and matches it against the contents.

Most of the rules are simple regex matching. The ones that are not are simple enough to define in the code.

Am I making any sense?

@basicallydan
Copy link
Owner

@M-Zuber Yes, you're making sense. However, I think using the file already in the repo will solve a different problem than the one being discussed previously in this thread. Let me refer to something I said earlier:

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?

The .gitignore in the repository already does the job of ensuring that you don't commit stuff that you shouldn't commit.

Forkability could still help here by warning people that a file has been committed even though it's supposed to be excluded by the .gitignore rules. But in that case, it means the file had to be explicitly added, or was added before the .gitignore file was created.

Am I making any sense? 😃

@M-Zuber
Copy link
Collaborator Author

M-Zuber commented Dec 27, 2015

You do make sense(or more culturally accurate pence 😛 ), but
I have started to have second thoughts about this whole premise.
If a project depends on a global .gitignore and it is not checked into the repo, that is a major smell 💩
On the other hand there is still something to add in this area - namely projects where a .gitignore was added in after development started. It is not so uncommon occurrence and forkability can help by bringing it to peoples attention.

Therefore I propose linting based off the actual .gitignore of the project.
Additionally it can be helpful in cases where the language is not supported

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

No branches or pull requests

2 participants