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

added .gitattributes file #130

Merged
merged 1 commit into from
Nov 23, 2016
Merged

added .gitattributes file #130

merged 1 commit into from
Nov 23, 2016

Conversation

OskarStark
Copy link
Member

@OskarStark OskarStark commented Jun 1, 2016

Closes #129

@@ -0,0 +1,4 @@
.editorconfig export-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Why not .*?

Copy link
Member Author

Choose a reason for hiding this comment

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

we can...

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@soullivaneuh
Copy link
Member

Could we add the documentation part?

.gitignore export-ignore
CONTRIBUTING.md export-ignore

/Resources/doc export-ignore
Copy link
Member

Choose a reason for hiding this comment

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

This is not always this path. Pleas use {{ docs_path }} instead.

Example:

cd {{ docs_path }} && sphinx-build -W -b html -d _build/doctrees . _build/html

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

What about tests?

@soullivaneuh
Copy link
Member

What about tests?

I would like #116 consensus before work on this.

@OskarStark
Copy link
Member Author

i got this error:

oskar.stark:/Volumes/development/workspaces/dev-kit (gitattributes-file)$ git rebase -i HEAD~2
}} is not a valid attribute name: project/.gitattributes:4
}} is not a valid attribute name: project/.gitattributes:4
}} is not a valid attribute name: project/.gitattributes:4
[losgelöster HEAD 4402d52] added .gitattributes file
 Date: Wed Jun 1 17:04:46 2016 +0200
 1 file changed, 4 insertions(+)
 create mode 100644 project/.gitattributes
Successfully rebased and updated refs/heads/gitattributes-file.
oskar.stark:/Volumes/development/workspaces/dev-kit (gitattributes-file)$

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

Maybe name it .gitattributes.twig ? It will avoid git recognizing it for dev-kit. Plus it's basically a Twig file, so…

@OskarStark
Copy link
Member Author

Maybe name it .gitattributes.twig ? It will avoid git recognizing it for dev-kit

sound good, what do you think @soullivaneuh ?

@soullivaneuh
Copy link
Member

@OskarStark Yes you can. In this case, the Twig variable name would be docs_path.

@OskarStark
Copy link
Member Author

so docs_path instead of {{ docs_path }} ?

@soullivaneuh
Copy link
Member

@OskarStark Well, it's a Twig file! So AFAIK you still need to use {{ }}! :trollface:

@OskarStark
Copy link
Member Author

OskarStark commented Jun 1, 2016

@OskarStark Well, it's a Twig file! So AFAIK you still need to use {{ }}! :trollface:

Exactly thats why i was asking! so now we should be fine! 👍

@OskarStark Yes you can. In this case, the Twig variable name would be docs_path.

I was wondering, because this was already done at this point ;-)

@@ -0,0 +1,4 @@
.* export-ignore
CONTRIBUTING.md export-ignore
Copy link
Member

Choose a reason for hiding this comment

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

*.md? Except if somebody want to keep a specific .md file...

Copy link
Member Author

Choose a reason for hiding this comment

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

*.md? Except if somebody want to keep a specific .md file...

i like the idea 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@OskarStark
Copy link
Member Author

i removed the indention, because we don't know the length of the docs path....

@soullivaneuh
Copy link
Member

i removed the indention, because we don't know the length of the docs path....

Hmm... or we can just specify the two cases (Resources/doc and docs).

I don't think git will cry if a ignored path does not exist, will do?

@OskarStark
Copy link
Member Author

no it won't but the indention is anyway bad if we add something like foo/bar/baz/loooonnnnggg tomorrow, the the whole file will be modified in the diff, it is the exact same problem with the double arrow align

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

What's wrong with the Twig variable?

@soullivaneuh
Copy link
Member

no it won't but the indention is anyway bad

Maybe, but I never seen a .gitattributes file without indentation. I don't know if a convention exists about that file...

@OskarStark
Copy link
Member Author

Maybe, but I never seen a .gitattributes file without indentation. I don't know if a convention exists about that file...

https://git-scm.com/book/uz/v2/Customizing-Git-Git-Attributes#Merge-Strategies

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

man gitattributes

That is, a pattern followed by an attributes list, separated by whitespaces. When the pattern matches the path in question, the attributes listed on the line are given to the path.

@OskarStark
Copy link
Member Author

What's wrong with the Twig variable?

Nothing i thought i should change {{ docs_path }} to docs_path without the curly braces... that confused me a few seconds 😄

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

What's wrong with the Twig variable?

No I meant why not use it, but I get it know, it was all about alignment

@OskarStark
Copy link
Member Author

OskarStark commented Jun 1, 2016

for example when we use this:

foo                      export-ignore
bar                      export-ignore
{{ docs_path}}           export-ignore

this would be the result:

foo                      export-ignore
bar                      export-ignore
/Resources/docs             export-ignore

you know what i mean?

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

you know what i mean?

Yes. Headaches ahead if we do that, as you explained.

@soullivaneuh
Copy link
Member

@OskarStark Looks good. Can you try it on a one of your forked sonata project?

After that, I would like @rande opinion for this before merge because I got a refusal from him last time and he broke my heart. 💔 😛

@greg0ire
Copy link
Contributor

greg0ire commented Jun 1, 2016

please

@rande
Copy link
Member

rande commented Jun 2, 2016

@soullivaneuh You should also ask the community ;)

@soullivaneuh
Copy link
Member

@soullivaneuh You should also ask the community ;)

Indeed, we might start a pool. I'll look for this week-end or next week. 👍

@OskarStark
Copy link
Member Author

@OskarStark Can you remove doc from .gitattributes for now? We can merge the rest and open a new issue to vote about that.

done @soullivaneuh

@greg0ire greg0ire added the RTM label Jun 28, 2016
@soullivaneuh
Copy link
Member

Thanks @OskarStark, this will be merged soon.

If you want to, you could open an issue to discuss about documentation ignore with vote.

@OskarStark
Copy link
Member Author

i agree with @rande that documentation could be helpful in some rare cases, so for me this is fine as a first step!

@core23
Copy link
Member

core23 commented Jun 30, 2016

Maybe we should remove phpunit.xml.dist and Makefile too

@OskarStark
Copy link
Member Author

sounds reasonable...

@greg0ire @soullivaneuh what do you think?

@@ -0,0 +1,2 @@
.* export-ignore
*.md export-ignore
Copy link
Member

Choose a reason for hiding this comment

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

When removing all Upgrade*.md and Readme.md files, we should also remove the docs.

Copy link
Member

Choose a reason for hiding this comment

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

When removing all Upgrade*.md and Readme.md files, we should also remove the docs.

Quite agree but let's discuss about this on another PR/issue! 👍

Then we should remove this entry.

Copy link
Member

Choose a reason for hiding this comment

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

@rande Talk about keeping the docs folder, not the md files. Plus, the README.md file is just a generic one with a link pointing to the online documentation.

So let's keep as is for now and launch a vote for docs folders.

@greg0ire
Copy link
Contributor

I agree!

@soullivaneuh
Copy link
Member

Maybe we should remove phpunit.xml.dist and Makefile too

Agree.

When removing all Upgrade*.md and Readme.md files, we should also remove the docs.

Quite agree but let's discuss about this on another PR/issue! 👍

@OskarStark
Copy link
Member Author

but, if we remove the phpunit.xml.dist shouldn't we remove the tests, too ❓

@soullivaneuh
Copy link
Member

soullivaneuh commented Jun 30, 2016

but, if we remove the phpunit.xml.dist shouldn't we remove the tests, too ❓

We don't care on this case, it's applies only for tarballs, not clones.

@core23
Copy link
Member

core23 commented Jun 30, 2016

but, if we remove the phpunit.xml.dist shouldn't we remove the tests, too ❓

We don't need the phpunit config outside the bundle, but some bundles depends on the Tests/*.php classes. Refs #179

@soullivaneuh
Copy link
Member

We don't need the phpunit config outside the bundle, but some bundles depends on the Tests/*.php classes. Refs #179

Yeah, but this should be removed, shouldn't be?

@core23
Copy link
Member

core23 commented Jun 30, 2016

Yeah, but this should be removed, shouldn't be?

Yes, but we can't remove them in all minor versions.

@soullivaneuh
Copy link
Member

Yes, but we can't remove them in all minor versions.

Already discussed on the concerned issue: This have to be fixed internally on minor versions. We don't care about external usage because it should not be used at all.

@core23
Copy link
Member

core23 commented Jul 15, 2016

We can now remove the Tests folder, it is excluded from the autoloader in all composer projects.

@core23
Copy link
Member

core23 commented Jul 24, 2016

Ping @OskarStark

@OskarStark
Copy link
Member Author

done @core23

@OskarStark
Copy link
Member Author

I did everything, can you please merge this @soullivaneuh ?

@OskarStark
Copy link
Member Author

Can we please merge this @soullivaneuh ?

@OskarStark
Copy link
Member Author

ping @soullivaneuh

@OskarStark OskarStark added the RTM label Aug 30, 2016
@core23
Copy link
Member

core23 commented Sep 15, 2016

Can we merge this @soullivaneuh ?

@core23
Copy link
Member

core23 commented Nov 4, 2016

Ping @soullivaneuh

@soullivaneuh soullivaneuh merged commit fd4151e into sonata-project:master Nov 23, 2016
@OskarStark OskarStark deleted the gitattributes-file branch November 23, 2016 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants