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 standard .gitignore to all exercises #193

Merged
merged 1 commit into from
Dec 8, 2016

Conversation

ashleygwilliams
Copy link
Contributor

@ashleygwilliams ashleygwilliams commented Aug 31, 2016

fixes #192 👾 ⚡

i used the content from https://github.com/github/gitignore/blob/master/Rust.gitignore

# Generated by Cargo
# will have compiled files and executables
/target/

# Remove Cargo.lock from gitignore if creating an executable, leave it for libraries
# More information here http://doc.crates.io/guide.html#cargotoml-vs-cargolock
Cargo.lock

@IanWhitney
Copy link
Contributor

I dig it. I'll let other folks weigh in if they want before merging.

# will have compiled files and executables
/target/

# Remove Cargo.lock from gitignore if creating an executable, leave it for libraries
Copy link
Member

Choose a reason for hiding this comment

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

Question, since we know for all exercises that they are libraries, might we remove this comment? Leaving it in might give the impression that the student needs to make a choice (when the choice has already been made, I believe)

Copy link
Member

Choose a reason for hiding this comment

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

And as a corollary, let's say that we do start ignoring Cargo.lock (as it appears we should).

Should we thus remove all Cargo.lock files from the repository in this case?
(That can go in a separate commit and can be a follow-up PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i left it in because i want it to simulate what someone might actually have to do if they create a project from scratch. if they make a project on GitHub, this is the auto-created .gitignore.

leaving the statement in there, 1) teaches students that the .gitignore should be different for executables vs libraries, 2) gives students the exercise of cleaning it up (similar to having to make the src directory and the lib.rs file), 3) does no harm if not modified.

so i see this as 2 learning opportunites vs 0 problems, so i'd leave it in.

that being said, if everyone disagrees with me i am happy to change the PR (this is not a mountain i'm going to die on 😆)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i actually missed the 3rd learning moment which is the student asking "what the heck is a Cargo.lock file" which is a good thing to learn sooner than later as well!

Copy link
Member

Choose a reason for hiding this comment

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

What if I proposed that we can preserve all the benefits described, and avoid the potential problem of the student wondering "so am I supposed to remove this or not?" by editing the comment to be something like "Since this is a library, we leave Cargo.lock in the .gitignore, but if it were an executable we would remove it" ?

(I am OK with either solution here, but I am usually in favor of making suggestions even if they end up being crazy)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i prefer to stick to convention over something custom here, but i'd be happy with what is already in the PR or the custom msg you described, leaning slightly towards what's already here

Copy link
Member

Choose a reason for hiding this comment

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

Unless anyone else expresses a strong opinion, it seems we can leave it as is.

@petertseng
Copy link
Member

Pre-emptively answering a question someone might ask for why did Haskell track remove their per-exercise gitignore in exercism/haskell#137 and exercism/haskell#138 while Rust track is adding them:

The removed Haskell gitignore files seemed accidental and ignored the file that the student should be submitting, causing confusion for students who stored their work in a Git repo. These gitignores added in this PR are intentional and provide a good baseline for students who wish to store their work in a Git repo, as target will now no longer be added.

I had a question about whether gitignore files work in any directory in the Git repo, but I searched and found that the answer is "yes". So that seems to be OK.

@petertseng
Copy link
Member

whether gitignore files work in any directory in the Git repo

Ah, that does make me realize that most people probably store all their exercises in a combined git repo containing multiple exercises rather than a separate repo for each exercise. It makes me wonder whether they might just make a gitignore for themselves in their own repo.

Nevertheless, if we give them one it will reduce work for them.

@petertseng
Copy link
Member

What about all exercises added after this PR is merged? Should there be a way to ensure that they have gitignore files too, if it is to be standard for all exercises?

Some combination of the following:

@ashleygwilliams
Copy link
Contributor Author

my thoughts, expressed in emoji:

✅ Travis CI fail if some exercise is missing its gitignore
✅ Add to README saying each exercise should have one
✅ A template dir for exercises, in the style of https://github.com/exercism/xjava/tree/master/exercises/_template and https://github.com/exercism/xkotlin/tree/master/exercises/_template
🙅 Do nothing, and reviewers of PRs containing new exercises have to remember to check for a gitignore :(

@ashleygwilliams
Copy link
Contributor Author

if others agree, i'll amend this PR to include those 3 things

@IanWhitney
Copy link
Contributor

I'm good with the discussion as it stands.

@petertseng
Copy link
Member

To be fair, we should have started the template dir as soon as our exercise dirs were something other than cargo new...

The challenge of the template dir is will it be too cumbersome if the template has to vary in many places depending on the exercise slug? Off the top of my head: The Cargo.toml file has to change (this is created by cargo new), the name of the file in tests has to change (this is not created by cargo new).

Basically my question is: even if we create a template dir, is there still too much work to do after having created it? And if so, is it even worth creating a template dir?


I believe a template dir has a few aspects we are not sure on yet. I would be totally OK saving the template dir for a separate PR with its own discussion. The other two points (Travis CI and README) have much fewer unspecified bits, and this little bit of work up front will already drastically reduce the chance of human error, and that's all I wanted since we are all human.

@ashleygwilliams
Copy link
Contributor Author

ashleygwilliams commented Sep 2, 2016

to do:

  • add note in README.md
  • add check in travis

@kytrinyx
Copy link
Member

kytrinyx commented Sep 2, 2016

A note for the future... once we have the ability to download global files for a track we may want to have a global gitignore instead of one for each exercise: exercism/cli#88 (I have no idea how long it will take to get this implemented, so it has absolutely no bearing on this pull request!)

@IanWhitney
Copy link
Contributor

@ashleygwilliams Anything I can do to help get this merged? I'm not sure if those todo items are things we should be doing, or that you intended to do.

@IanWhitney
Copy link
Contributor

Thoughts on closing this one? I'm unclear on the state of this PR. If it has useful things in it, then I would like to get it merged.

@petertseng
Copy link
Member

petertseng commented Dec 7, 2016

There would be nothing wrong really with merging this one as-is - it's just that then there would be nothing enforcing that the same .gitignore gets added to all other exercises, so it is possible that our exercises would come to be in an inconsistent state (some with gitignore, some without) if a future contributor adds an exercise and forget to add the gitignore (as we are human and wont to do).

Then again, they already are:

$ find -name .gitignore
./exercises/pascals-triangle/.gitignore
./exercises/triangle/.gitignore
./exercises/grains/.gitignore
./exercises/bowling/.gitignore

The strength of my opinion is a 2/10 here, so really I'm happy to defer to anyone with a stronger.

@IanWhitney IanWhitney merged commit d79a9c8 into exercism:master Dec 8, 2016
@IanWhitney
Copy link
Contributor

Thanks, @ashleygwilliams !

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.

include a gitignore file in exercises
4 participants