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

EZP-26801: User specific files should not appear in .gitignore #148

Merged
merged 1 commit into from
Jan 25, 2017

Conversation

clash82
Copy link
Contributor

@clash82 clash82 commented Dec 16, 2016

https://jira.ez.no/browse/EZP-26801

We want platinum medal in SensioLabsInsight! ;-)

All user specific files from now on should be kept in the global .gitignore file
located in the user's local environment. To do so you have to inform Git which
files should be excluded globally (for every Git repository).

Here is a short instruction about how to exclude selected files globally:

- create a new .gitignore file in your home folder:
  touch ~/.gitignore

- add some exclusions, eg:
  /nbproject/
  /.idea/

- inform Git where it should start looking for .gitignore file:
  git config --global core.excludesfile ~/.gitignore

More details about excluding specific file on your local environment can be found
in the official GitHub article: https://help.github.com/articles/ignoring-files/

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

ok.

But.. maybe you can make sure to add a internal comment here referring to how people can add these to global their git settings? They'll end up looking in here for it, so natural place to point out where they need to go.

@clash82 clash82 force-pushed the ezp-26801 branch 2 times, most recently from 996e0be to e9d8c3f Compare December 19, 2016 09:31
@clash82
Copy link
Contributor Author

clash82 commented Dec 19, 2016

@andrerom good idea, added (both in pull request description and commit message).

@yannickroger
Copy link
Contributor

Can we the message to user as a comment in the .gitignore file? So if someone looks inside the file to add the rules, he/she will be led to the right way to do it.

@clash82
Copy link
Contributor Author

clash82 commented Dec 20, 2016

Can we the message to user as a comment in the .gitignore file?

I like it, added.

@emodric
Copy link
Contributor

emodric commented Dec 20, 2016

What's the difference of files being specified here or in global .gitignore? If the repo rule is that these files are ignored, then they should be included in local ignore file. With this PR, I would have to tell each and every of my member of my team (which can span 3 companies) to put .idea folder in their .gitignore on their development machines?

Yes, I know I can add it back myself, but I'm talking about sensible defaults here.

@yannickroger
Copy link
Contributor

yannickroger commented Dec 21, 2016

I guess the Symfony good practice (since the goal here is to have a nicer medal on sensio insight) is about not adding user specific information that have nothing to do with the code itself.

If you are using PHP Storm for example (following your example), you are probably using it for more than one project, so having a global config sounds quite smart. If you want to document it for developers, you should tell them to do it while installing their IDE.

<troll joke>
    Or you use an IDE/text editor that won't leave stuff in the project folder 😄 
</troll joke>

@alongosz
Copy link
Member

I'm gonna agree with what SensioLabsInsight suggests.
Ignoring globally IDE files should be a part of every Developer's environment initial setup.
It just seems fair to me - there are many IDEs, so it is impossible to include them all in local .gitignore.

andrerom
andrerom previously approved these changes Dec 21, 2016
Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

Appeoved with example.

# files should be excluded globally (for every Git repository).
#
# More details about excluding specific file on your local environment can be found
# in the official GitHub article: https://help.github.com/articles/ignoring-files/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add an example for how to exclude at least idea and netbeans files so it's more clear, and so most of us can avoid having to check the link and deduct it ourselves ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated... I won't say anything :)

@emodric
Copy link
Contributor

emodric commented Dec 22, 2016

since the goal here is to have a nicer medal on sensio insight

The goal is to have sensible defaults which make sure that files that should not be in repo don't end up in repo, without me having to going from person to person and telling them to add the config to global .gitignore. What if some of them work on private projects and want the .idea folder in the repo in order not to loose the project setup for PhpStorm? Getting a platinum medal on a service 99% of users don't even know about is a wild goose chase if it hinders usability.

I'm gonna agree with what SensioLabsInsight suggests.
Ignoring globally IDE files should be a part of every Developer's environment initial setup.

Maybe, but in large teams you cannot expect and make sure that each and every member of the team, especially those inexperienced and remote ones, have it setup correctly.

It just seems fair to me - there are many IDEs, so it is impossible to include them all in local .gitignore.

That's overreaching. With PhpStorm you basically covered 99% of them, so I don't think there is need to.

@clash82
Copy link
Contributor Author

clash82 commented Dec 22, 2016

Hi @emodric

What if some of them work on private projects and want the .idea folder in the repo in order not to loose the project setup for PhpStorm?

That's why I added link to the official GitHub article where you can find more details on how to exclude files for different use cases. In your example this will be https://help.github.com/articles/ignoring-files/#explicit-repository-excludes

@emodric
Copy link
Contributor

emodric commented Dec 22, 2016

That's why I added link to the official GitHub article where you can find more details on how to exclude files for different use cases. In your example this will be https://help.github.com/articles/ignoring-files/#explicit-repository-excludes

That's just a dirty and backwards hack. Instead of having specific things in .gitignore in repos where we WANT them, let's just force users to use global config, and then if they need it, let them hack into .git folder to make it possible to have a thing they have by default without global config.

It doesn't feel wrong to you to add an 18 line comment (which no one will read) explaining something to users, just for the sake of a platinum medal, instead of having 2 lines in .gitignore which will solve all issues without users having to think about it and hack .git folder?

@andrerom
Copy link
Contributor

@emodric your point have been made, but maybe you should use some of that heat to discussion to symfony community so they adjust their rules instead.

@emodric
Copy link
Contributor

emodric commented Dec 22, 2016

It's not heat, it's a discussion ;) My point is only that we don't have to blindly follow Symfony community rules.

@clash82
Copy link
Contributor Author

clash82 commented Jan 4, 2017

@andrerom can we merge it?

@andrerom andrerom dismissed their stale review January 4, 2017 12:16

Disagreement with user should be resolved first, one option is reaching out to symfony and getting them to remove this from their recommendations.

@emodric
Copy link
Contributor

emodric commented Jan 9, 2017

It is not my intention to block this PR, I was just expressing an alternate opinion to be considered. I'm sure those who want .idea in their .gitignore will just revert this PR and add it back, so don't mind me and feel free to merge :)

@clash82
Copy link
Contributor Author

clash82 commented Jan 24, 2017

@andrerom disagreement with user seems to be resolved. This PR was created as a suggestion to use "best practises" recommended by Insight. If there is not approval for merge then I will just close it.

@andrerom andrerom merged commit e6cc0f7 into ezsystems:master Jan 25, 2017
@clash82 clash82 deleted the ezp-26801 branch January 25, 2017 09:12
andrerom pushed a commit that referenced this pull request Dec 2, 2020
* [Platform.sh]: Added ElasticSearch support

* fixup! [Platform.sh]: Added ElasticSearch support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants