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

Migrate file structure to PSR-4 #716

Closed
wants to merge 1 commit into from
Closed

Migrate file structure to PSR-4 #716

wants to merge 1 commit into from

Conversation

skymeyer
Copy link
Contributor

Initial proposal:

  • move lib/Elastica to lib
  • move test/lib/Elastica/Test to test/lib

Let me now if there are other opinions in directory restructuring.

Resolves #715

@fprochazka
Copy link
Contributor

@skymeyer great 👍

@ruflin
Copy link
Owner

ruflin commented Nov 2, 2014

This will only have an affect on all the people using Elastica without composer, right?

@skymeyer
Copy link
Contributor Author

skymeyer commented Nov 2, 2014

That's correct @ruflin. It will also affect all open PR's so we need to coordinate when you feel is the best time to pull this change in. Maybe best pulling this in just after next planned release ? I need to rebase it anyway as this PR will render invalid every time a new change is pulled into master on the previous directory structure.

@ruflin
Copy link
Owner

ruflin commented Nov 3, 2014

I'm thinking about releasing it with elasticsearch 1.4.0 which is still in beta1. I assume there are going to be some other BC breaks. But I don't really know when the 1.4.0 release is planned.

@skymeyer
Copy link
Contributor Author

skymeyer commented Nov 6, 2014

@ruflin 1.4.0 is out since yesterday, let me know when you want the rebase.

@ruflin
Copy link
Owner

ruflin commented Nov 11, 2014

@skymeyer I will let you know as soon as I find the time (hopefully at the end of this week).

@ruflin
Copy link
Owner

ruflin commented Nov 16, 2014

@dbu Can you have a look at this. I assume there is no affect on the Symfony bundle, but lets better be sure then sorry.

@merk
Copy link
Collaborator

merk commented Nov 16, 2014

FOSElasticaBundle wont be affected by this. The composer autoloader will handle it all.

"Elastica\\": "lib/Elastica/",
"Elastica\\Test\\": "test/lib/Elastica/Test/"
"Elastica\\": "lib/",
"Elastica\\Test\\": "test/lib/"
Copy link
Contributor

Choose a reason for hiding this comment

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

most libraries i see use just tests/ not followed by "lib" for this.

btw, it seems to be a convention that tests are in the namespace "Tests". if there is "Test", i would expect it to provide testing helpers that i could use for my tests as a library user (like a mock elasticsearch client or something). but changing that has a lot more potential for BC breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree yes. @ruflin we can handle this in a separate issue if you would like to "cleanup" the tests as @dbu mentioned. Not highly needed, just a bit more work.

@dbu
Copy link
Contributor

dbu commented Nov 16, 2014

seems sane to me. as long as people where using composer, they will not notice what is happening here. if they don't, they really should - but will have to adjust the path in their autoloading.

in the cmf, we did a minor version bump when switching to psr-4.

@ruflin
Copy link
Owner

ruflin commented Dec 6, 2014

@skymeyer Sorry that we didn't progress here yet. I would like to get some of the open pull requests in first. I hope this is ok for you.

@ruflin
Copy link
Owner

ruflin commented Jun 1, 2015

After lots of internal changes I think Elastica is now finally ready to move the structure to PSR-4. @skymeyer I'm not sure if it makes sense to merge all changes or "just" to redo it from scratch?

@dbu
Copy link
Contributor

dbu commented Jun 1, 2015

i would recommend to do it in a fresh PR. its just the changes to composer.json and moving all files up to remove the unnecessary namespace folders. doing it from current master also keeps some chances that existing pull requests won't break.

@skymeyer
Copy link
Contributor Author

skymeyer commented Jun 2, 2015

Better to redo this yes @ruflin - I'll send a new one.

@ruflin
Copy link
Owner

ruflin commented Jun 2, 2015

👍

@ruflin
Copy link
Owner

ruflin commented Jul 18, 2016

Closing this one as this is better done in a new PR.

@ruflin ruflin closed this Jul 18, 2016
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.

Migrate file structure to PSR-4
5 participants