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

Removed Translator and various other improvements and corrections for version 2 of the component framework #40

Merged
merged 55 commits into from Jun 26, 2014

Conversation

cordoval
Copy link
Contributor

Q A
Bug Fix? yes
New Feature? yes
BC Breaks? expected
Deprecations? translator related
Tests Pass? yes
Fixed Tickets none
License Aura/MIT
Doc PR no

Sent using Gush

Description

This PR was started after today's conversation on IRC, we synced with Hari and Paul to remove translator as a dependency from this component. There were some additional cleanups, moving to PSR-4, updating the readme, making the tests pass, correcting some typos on the tests, adding an autoload.php, creating a factory for the service instantiation and registry initialization and some other minor issues.

todo:

  • update README.md with new structure

@cordoval
Copy link
Contributor Author

@pmjones @harikt please run a first review, i still need to fix the Factory and also have a sort of registry i guess built inside the class

@cordoval
Copy link
Contributor Author

@pmjones @harikt any early reviews?

@cordoval cordoval changed the title [WIP] develop-2 with harikt and cordoval Removed Translator and various other improvements and corrections for version 2 of the component framework May 26, 2014
@cordoval
Copy link
Contributor Author

@pmjones @harikt ok that is it, thanks for your friendship 😊

@cordoval
Copy link
Contributor Author

@harikt @pmjones could you please actually set scrutinizer for this repo? thanks

@pmjones
Copy link
Member

pmjones commented Jun 16, 2014

Scrutinizer added just now per your request!

@cordoval
Copy link
Contributor Author

thanks @pmjones

@cordoval
Copy link
Contributor Author

@pmjones https://scrutinizer-ci.com/g/auraphp/Aura.Filter/ i see it however i cannot trigger it for develop-2 branch in which i am working

@cordoval
Copy link
Contributor Author

there is a setting to trigger it for all builds in all branches

@pmjones
Copy link
Member

pmjones commented Jun 16, 2014

updated to set develop-2 as the main branch

@cordoval
Copy link
Contributor Author

@pmjones i think it sounds to me this is good to merge now. I see other components don't have common/config folder, should i remove it?

@harikt
Copy link
Member

harikt commented Jun 16, 2014

config folder is good. So don't remove it.

Thanks

@cordoval
Copy link
Contributor Author

i will not, but i believe its place is elsewhere in another package

@cordoval
Copy link
Contributor Author

@harikt @pmjones ok Paul said he will review tomorrow. Other fixes or improvements can come later.

@harikt
Copy link
Member

harikt commented Jun 16, 2014

One thing I like in v2 is usage of

$email = new Aura\Filter\Email('email@example.com');
$email->isValid();

like interface. I am not sure whether this is possible currently.

But just a wishlist . The idea is usage of filters in DDD as Mathias Verras mentioned in some slides.

@cordoval
Copy link
Contributor Author

Please open a ticket with that wish maybe we can implement but it is not related to this PR. 😊 Is actually a very good idea, to go strong in DDD with aura libraries.

@@ -2,12 +2,8 @@ language: php
php:
- 5.4
- 5.5
- hhvm
Copy link
Member

Choose a reason for hiding this comment

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

hhvm not working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just don't think we should test against something that fails and that is not fully working, we either remove it or remove the allow_failtures cc @pmjones

@harikt
Copy link
Member

harikt commented Jun 18, 2014

Hey @cordoval could you add a branch alias?

"branch-alias": {
    "dev-develop-2": "2.0.x-dev"
}

See https://github.com/auraphp/Aura.Web/blob/develop-2/composer.json#L33-L35

@cordoval
Copy link
Contributor Author

@pmjones please review and merge. What comes after this? What other component needs help? Or should we move cordoval/bc-analyzer into auraphp? so we can pound on it?

@harikt
Copy link
Member

harikt commented Jun 19, 2014

@cordoval could you have a look into the https://github.com/auraphp/Aura.Filter/issues?state=open #31 , #32, #34 , , #43

And your thoughts how to improve?

@cordoval
Copy link
Contributor Author

ok i can but let's close this chapter PR please

@harikt
Copy link
Member

harikt commented Jun 19, 2014

Sure. Let Paul review things. Please give him sometime.

Aura.Session v2 need some help / improvements. I don't really know what all
are planned though / good.

@pmjones
Copy link
Member

pmjones commented Jun 26, 2014

I'll take this as-is and review later. Many thanks for your work, gentlemen.

pmjones pushed a commit that referenced this pull request Jun 26, 2014
@pmjones pmjones merged commit 6581711 into auraphp:develop-2 Jun 26, 2014
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.

None yet

4 participants