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

OPENEUROPA-777: Make sure component dependencies are as relaxed as possible. #9

Merged
merged 11 commits into from
Sep 21, 2018

Conversation

aritomelo
Copy link
Contributor

Set Minimal minor version.

And also added decimal numer to keep the consistency.

… it use the minimal minor version.

And also added decimal numer on version to keep the consistency.
Copy link
Member

@pfrenssen pfrenssen left a comment

Choose a reason for hiding this comment

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

Looks good, but needs a few small tweaks.

.drone.yml Outdated Show resolved Hide resolved
composer.json Outdated
},
"require-dev": {
"phpunit/phpunit": "~5||~6.0",
"openeuropa/code-review": "~0.3"
"phpunit/phpunit": "~5.0||~6.0",
Copy link
Member

Choose a reason for hiding this comment

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

Let's also update this to remove the double pipe, this is not according to the Composer specification. Let's make it ~5.0|~6.0.

@voidtek voidtek force-pushed the OPENEUROPA-777 branch 6 times, most recently from f19cdf9 to e2204bf Compare September 11, 2018 12:28
@voidtek
Copy link
Member

voidtek commented Sep 12, 2018

@richardcanoe, I revert your commit according to my comment #9 (comment)

dxvargas
dxvargas previously approved these changes Sep 12, 2018
richardcanoe
richardcanoe previously approved these changes Sep 12, 2018
Copy link
Contributor

@richardcanoe richardcanoe left a comment

Choose a reason for hiding this comment

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

Happy with Voidtek's changes 👍

@voidtek voidtek dismissed stale reviews from richardcanoe and dxvargas via 00c73be September 12, 2018 12:25
richardcanoe
richardcanoe previously approved these changes Sep 12, 2018
Copy link
Contributor

@richardcanoe richardcanoe left a comment

Choose a reason for hiding this comment

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

👍

dxvargas
dxvargas previously approved these changes Sep 12, 2018
@dxvargas dxvargas dismissed stale reviews from richardcanoe and themself via 423eaf2 September 13, 2018 13:20
richardcanoe
richardcanoe previously approved these changes Sep 13, 2018
Copy link
Member

@pfrenssen pfrenssen left a comment

Choose a reason for hiding this comment

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

I would like to see a useful description of the matrix options and to not add the .editorconfig file. This does not appear to be complete, and is out of scope for the issue.

.drone.yml Outdated
@@ -33,3 +33,6 @@ matrix:
IMAGE_PHP:
- fpfis/httpd-php-dev:5.6
- fpfis/httpd-php-dev:7.1
COMPOSER_BOUNDARY:
- '--prefer-lowest'
- ''
Copy link
Member

Choose a reason for hiding this comment

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

I'm don't like how a command option and an empty string are being used as matrix options. This is taking a shortcut that makes it more difficult to understand.

The original approach that was proposed by Gregory is better: he proposed to set the boundary options to "highest" and "lowest" and limit the builds by using the "when" parameter. This makes it clear what is going on. In addition to this the steps in the pipeline were descriptively named.

See here for an example of how this can be done in a clean way: https://github.com/openeuropa/code-review/blob/d4e105a788835ef851c3816eceffb5c26ba34517/.drone.yml

.editorconfig Outdated
[*]
end_of_line = LF
indent_style = space
indent_size = 4
Copy link
Member

@pfrenssen pfrenssen Sep 17, 2018

Choose a reason for hiding this comment

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

This is setting the indenting to 4 spaces for all the files in the project. But for example I see that the .drone.yml file is using an indent of 2 spaces. This is in conflict with what is being proclaimed here.

I would suggest to take this out of this PR for now, it is out of scope and needs additional work. This can best be handled in a dedicated issue.

@pfrenssen
Copy link
Member

This is looking good now, but one of my earlier remarks turned out to be incorrect as was pointed out by @dxvargas in one of the sister issues. I suggested to change the "or" clause in the PHPUnit version string from a double pipe to a single pipe, but in fact the double pipe is the preferred symbol, as documented in Versions and constraints - version range. The single pipe is only supported for backwards compatibility reasons.

I will make this small correction and approve the PR.

Copy link
Member

@ademarco ademarco left a comment

Choose a reason for hiding this comment

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

Looking good!

@ademarco ademarco merged commit 589c825 into master Sep 21, 2018
@ademarco ademarco deleted the OPENEUROPA-777 branch September 21, 2018 11:16
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.

6 participants