Skip to content

Updated .gitignore to be more specific #342

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rynibami
Copy link

Hi, I want to improve the rcgen crate and noticed that certain items were not included into the .gitignore. So I thought updating this file would be a great start of introducing myself and start working on the project (if you would have me). 😄

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Maybe a little bit overkill, but seems harmless :-) Thanks!

@djc
Copy link
Member

djc commented May 16, 2025

I'd prefer something smaller/more focused. Do you have an actual file that you need ignored in your own setup?

@Rynibami
Copy link
Author

Yes, I'm building on MacOS with VSCode (and have used Rust Rover in the past). So there are a few files / directories I like to mask out for git.

@djc
Copy link
Member

djc commented May 17, 2025

Okay, can you get rid of the three-line section headers and trim this to just the stuff that you have a direct/short-term need for?

@Rynibami Rynibami force-pushed the update-gitignore branch 2 times, most recently from 893f765 to 55cfe2a Compare May 17, 2025 07:13
.gitignore Outdated
/target

# Secret
certs/
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you add a trailing newline?

@est31
Copy link
Member

est31 commented May 17, 2025

I don't think we should be adding such extensive changes. editor specific files, backup files, etc should go into the user-local gitignore, not into each project's gitignore. project gitignores should get stuff that is either specific to the language/technology (like build artifact dir) or to the individual project (say a dir for generated certificates, if the tool generates certificates).

@est31
Copy link
Member

est31 commented May 17, 2025

From https://git-scm.com/docs/gitignore

Patterns which a user wants Git to ignore in all situations (e.g., backup or temporary files generated by the user’s editor of choice) generally go into a file specified by core.excludesFile in the user’s ~/.gitconfig.

Note that in the past I've also done this extend gitignore game, but eventually I realized that user-specific gitignores are better for editor specific stuff.

@Rynibami
Copy link
Author

I don't think we should be adding such extensive changes. editor specific files, backup files, etc should go into the user-local gitignore, not into each project's gitignore. project gitignores should get stuff that is either specific to the language/technology (like build artifact dir) or to the individual project (say a dir for generated certificates, if the tool generates certificates).

@est31, can you specify what items you would like to update / remove? Following your strategy, the original file would also remove the .idea rule.

@est31
Copy link
Member

est31 commented May 17, 2025

yeah probably we should also remove .idea as well, and call it done. The comments are also useful. Outside of that I don't see a need to change .gitignore.

@Rynibami
Copy link
Author

@est31, I'll return the .gitignore to what it mostly was (and keep some comments). Shall I put a small description (and reference to this PR as well) that describes / explains this choice? This will prevent future discussions around this topic.

@Rynibami Rynibami force-pushed the update-gitignore branch from 55cfe2a to 90c3975 Compare May 17, 2025 17:07
Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for filing!

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.

4 participants