Add .gitattributes to exclude dev files from exports

[reedy]

No description provided.

[clue]

@reedy Thank you for looking into this, the changes LGTM technically, but may I ask you to add your motivation for this change for future reference? 👍

[reedy]

Yup! It prevents those files being included when people include the repo as a dependancy via composer, or git export

Fairly standard practice across most repos these days

• https://madewithlove.be/gitattributes/
• https://alexbilbie.com/2012/11/exclude-objects-with-gitattributes/
• http://www.pixelite.co.nz/article/using-git-attributes-exclude-files-your-release/

[clue]

@reedy Thank you for providing some links for future reference! Not everybody may have the same information, so I think it's always useful to include something like this. 👍

Technically, I've argued against this change in the past because I don't think "excluding files from exports" is the right thing to do semantically (after all, these files are supposed to be part of the git repository archive).

But for practical reasons I'm okay with these changes as-is: Composer uses the git archive feature (which is what GitHub uses to offer tarballs) when downloading any dependencies, so excluding files that aren't usually used in vendor files makes sense to noticeably reduce the download size for each package.

Arguably, the vendor folder could still be cleaned up before deploying to production, but this doesn't solve the downloading/bandwidth issues.

To sum this up, I think it's the best solution we have right now and the downsides are somewhat negligible, so let's get this in

Do you plan to backport this to the 2.x branch and apply this to ReactPHP's other components? 👍

[reedy]

Do you plan to backport this to the 2.x branch and apply this to ReactPHP's other components? 👍

I hadn't planned to ("we" only use this component over at Wikimedia), but if it's welcome I don't mind doing so

[reedy]

#154 does 2.x on this repo

[clue]

@reedy Thanks for filing all other PRs! Instead of replying to every single one, may I ask you to update the title and description, squashing the commits into a single one (minor nit: entries should be ordered by file name to not provoke any minor PRs addressing this again in the future) and maybe provide a link to this PR here (again for future reference for somebody who doesn't know how these are related). Thank you!

[WyriHaximus]

Technically, I've argued against this change in the past because I don't think "excluding files from exports" is the right thing to do semantically (after all, these files are supposed to be part of the git repository archive).

Absolutely disagree, we as maintainers should make sure our packages only contain what is needed to run this package. When a user wants everything they can still do --prefer-source when running composer install and get everything. Distribution packages should be slim IMHO. But glad to see this happening 👍

[WyriHaximus]

Looks great, but can you make sure the commit message matches the PR title e.g. Add .gitattributes to exclude dev files from exports. I also understand that this is some manual labour to do it for all PR's you made, so if you rather not let me know and I happily accept it in the current state as I'm already super happy with it 👍 .

[reedy]

Looks great, but can you make sure the commit message matches the PR title e.g. Add .gitattributes to exclude dev files from exports. I also understand that this is some manual labour to do it for all PR's you made, so if you rather not let me know and I happily accept it in the current state as I'm already super happy with it 👍 .

The eternal question of whether it'd be quicker to write a script to do it, or change it manually...

[WyriHaximus]

Looks great, but can you make sure the commit message matches the PR title e.g. Add .gitattributes to exclude dev files from exports. I also understand that this is some manual labour to do it for all PR's you made, so if you rather not let me know and I happily accept it in the current state as I'm already super happy with it 👍 .

The eternal question of whether it'd be quicker to write a script to do it, or change it manually...

My money is on manual, assuming you'd only use it for this set of PR's...but it could have reuse value 🤐

[reedy]

$ cat fixcommitsummary.sh 
#!/bin/bash

for i in ~/reactphp*; do
    cd $i
    echo $i
    git checkout gitattributes
    git commit --amend -m "Add .gitattributes to exclude dev files from exports"
    git push origin gitattributes --force
done

Definitely took less time to write that than doing them manually, using pasted commands or up/enter etc

[WyriHaximus]

$ cat fixcommitsummary.sh 
#!/bin/bash

for i in ~/reactphp*; do
    cd $i
    echo $i
    git checkout gitattributes
    git commit --amend -m "Add .gitattributes to exclude dev files from exports"
    git push origin gitattributes --force
done

Definitely took less time to write that than doing them manually, using pasted commands or up/enter etc

Awesome! Can confirm you push to all the PR's, except for this one xD

[reedy]

$ cat fixcommitsummary.sh 
#!/bin/bash

for i in ~/reactphp*; do
    cd $i
    echo $i
    git checkout gitattributes
    git commit --amend -m "Add .gitattributes to exclude dev files from exports"
    git push origin gitattributes --force
done

Definitely took less time to write that than doing them manually, using pasted commands or up/enter etc

Awesome! Can confirm you push to all the PR's, except for this one xD

Should be done already

[WyriHaximus]

$ cat fixcommitsummary.sh 
#!/bin/bash

for i in ~/reactphp*; do
    cd $i
    echo $i
    git checkout gitattributes
    git commit --amend -m "Add .gitattributes to exclude dev files from exports"
    git push origin gitattributes --force
done

Definitely took less time to write that than doing them manually, using pasted commands or up/enter etc

Awesome! Can confirm you push to all the PR's, except for this one xD

Should be done already

It's not showing: https://github.com/reactphp/promise/pull/153/commits

(Not saying that you did or didn't, only observing :).)

I'll assign milestones, labels, and reviewers to the other PR's as well. Will also kick the failing builds so everything is green 👍

[reedy]

$ cat fixcommitsummary.sh 
#!/bin/bash

for i in ~/reactphp*; do
    cd $i
    echo $i
    git checkout gitattributes
    git commit --amend -m "Add .gitattributes to exclude dev files from exports"
    git push origin gitattributes --force
done

Definitely took less time to write that than doing them manually, using pasted commands or up/enter etc

Awesome! Can confirm you push to all the PR's, except for this one xD

Should be done already

It's not showing: https://github.com/reactphp/promise/pull/153/commits

(Not saying that you did or didn't, only observing :).)

I'll assign milestones, labels, and reviewers to the other PR's as well. Will also kick the failing builds so everything is green 👍

Ooh. Apparently I never cloned that locally...

[WyriHaximus]

$ cat fixcommitsummary.sh 
#!/bin/bash

for i in ~/reactphp*; do
    cd $i
    echo $i
    git checkout gitattributes
    git commit --amend -m "Add .gitattributes to exclude dev files from exports"
    git push origin gitattributes --force
done

Definitely took less time to write that than doing them manually, using pasted commands or up/enter etc

Awesome! Can confirm you push to all the PR's, except for this one xD

Should be done already

It's not showing: https://github.com/reactphp/promise/pull/153/commits
(Not saying that you did or didn't, only observing :).)
I'll assign milestones, labels, and reviewers to the other PR's as well. Will also kick the failing builds so everything is green 👍

Ooh. Apparently I never cloned that locally...

Yeah the branch name kinda gave that away 😂 . Thanks again, the rest of them should also be merged soon enough 🎉

[clue]

Technically, I've argued against this change in the past because I don't think "excluding files from exports" is the right thing to do semantically (after all, these files are supposed to be part of the git repository archive).

Absolutely disagree, we as maintainers should make sure our packages only contain what is needed to run this package. When a user wants everything they can still do --prefer-source when running composer install and get everything. Distribution packages should be slim IMHO. But glad to see this happening +1

I don't think this is going to lead us anywhere, but I would argue that this package only contains what is needed to run this package (which involves the whole software release life cycle and not just runtime environment). If we're only talking about the runtime environment, then arguably something needs to take care of removing certain files from the release artifact. In this case, one could also argue that the README and CHANGELOG are not needed either, but it looks like we agree that this should be part of the release artifact anyway. That's why I would argue that the release archive is semantically not the best place to do this – from a pragmatic perspective it's just what happens to be available for Composer to use. Either way, glad this has been merged because it's the best option that's currently available :-)

[WyriHaximus]

Technically, I've argued against this change in the past because I don't think "excluding files from exports" is the right thing to do semantically (after all, these files are supposed to be part of the git repository archive).

Absolutely disagree, we as maintainers should make sure our packages only contain what is needed to run this package. When a user wants everything they can still do --prefer-source when running composer install and get everything. Distribution packages should be slim IMHO. But glad to see this happening +1

I don't think this is going to lead us anywhere

It won't, that's why I consider this a good compromise :). Although I'm personally considering dropping everything but src, composer.json and LICENSE from my packages dist files. But that's a discussion for another time and another iteration :).

[reedy]

Technically, I've argued against this change in the past because I don't think "excluding files from exports" is the right thing to do semantically (after all, these files are supposed to be part of the git repository archive).

Absolutely disagree, we as maintainers should make sure our packages only contain what is needed to run this package. When a user wants everything they can still do --prefer-source when running composer install and get everything. Distribution packages should be slim IMHO. But glad to see this happening +1

I don't think this is going to lead us anywhere

It won't, that's why I consider this a good compromise :). Although I'm personally considering dropping everything but src, composer.json and LICENSE from my packages dist files. But that's a discussion for another time and another iteration :).

composer.json isn't even actually needed. But that definitely causes arguments in various places :D

[WyriHaximus]

Technically, I've argued against this change in the past because I don't think "excluding files from exports" is the right thing to do semantically (after all, these files are supposed to be part of the git repository archive).

Absolutely disagree, we as maintainers should make sure our packages only contain what is needed to run this package. When a user wants everything they can still do --prefer-source when running composer install and get everything. Distribution packages should be slim IMHO. But glad to see this happening +1

I don't think this is going to lead us anywhere

It won't, that's why I consider this a good compromise :). Although I'm personally considering dropping everything but src, composer.json and LICENSE from my packages dist files. But that's a discussion for another time and another iteration :).

composer.json isn't even actually needed. But that definitely causes arguments in various places :D

Cool didn't know that! Is is like the committing composer.lock discussions? :P