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

Update nikolaposa/version to version 4 #73

Merged
merged 8 commits into from
Mar 18, 2021
Merged

Update nikolaposa/version to version 4 #73

merged 8 commits into from
Mar 18, 2021

Conversation

kevin-lot
Copy link
Contributor

@dontub

I have a question.
Why do you keep the old version of nikolaposa/version (v2 and v3) ?
dependencies of nikolaposa/version are:

  • v4 => beberlei/assert and php ^7.2 (so you can upgrade your package to php 7.2, too)
  • v3 => php ^7.2 (so you can upgrade your package to php 7.2, too)
  • v2 => php ^5.5 (this time, it's shivas/versionning that locks to php 7.1)

So, to put it in a nutshell,
If you want to keep shivas/versionning compatible with php 7.1, version 2 of nikolaposa/version is mandatory.
Without it, only version 4 of nikolaposa/version is required.

@dontub
Copy link
Collaborator

dontub commented Feb 14, 2021

I see no need to keep compatibility with PHP 7.1. When updating to nikolaposa/version v4, keeping compatibility with v2/v3 would make the code unnecessarily more complex.

@kevin-lot
Copy link
Contributor Author

So I removed v2 and v3 of nikolaposa/version.
I tried to update travis too.

Copy link
Collaborator

@dontub dontub left a comment

Choose a reason for hiding this comment

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

VersionBumpCommand needs to be changed, too.

Formatter/GitDescribeFormatter.php Outdated Show resolved Hide resolved
Formatter/GitDescribeFormatter.php Outdated Show resolved Hide resolved
Formatter/GitDescribeFormatter.php Outdated Show resolved Hide resolved
Formatter/GitDescribeFormatter.php Outdated Show resolved Hide resolved
Formatter/GitDescribeFormatter.php Outdated Show resolved Hide resolved
Formatter/GitDescribeFormatter.php Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@dontub dontub mentioned this pull request Feb 22, 2021
@kevin-lot
Copy link
Contributor Author

kevin-lot commented Feb 24, 2021

Sorry about the "else" and "elseif".
This is a job conditionning, in my company, we don't allow to code with "else" or "elseif". 😄

@dontub
Copy link
Collaborator

dontub commented Mar 3, 2021

I'd suggest to add a method private function transformPreRelease(string $preRelease): ?string { ... }. This would allow to immediately return and thus to avoid the elses. WDYT?

Do you have time to update VersionBumpCommand, too?

@kevin-lot
Copy link
Contributor Author

Do you have time to update VersionBumpCommand, too?

Done.

@kevin-lot
Copy link
Contributor Author

kevin-lot commented Mar 3, 2021

I'd suggest to add a method private function transformPreRelease(string $preRelease): ?string { ... }. This would allow to immediately return and thus to avoid the elses. WDYT?

I have to take a look to do with methods.
By the way, I think this is a good idea.

Command/VersionBumpCommand.php Outdated Show resolved Hide resolved
@kevin-lot kevin-lot requested a review from dontub March 17, 2021 07:55
@dontub dontub merged commit 044c047 into shivas:master Mar 18, 2021
@dontub
Copy link
Collaborator

dontub commented Mar 18, 2021

Thanks for contribution @kevin-lot!

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.

2 participants