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

Refactor, improvement and PHP 8.1 only #7

Merged
merged 8 commits into from
Oct 7, 2022
Merged

Refactor, improvement and PHP 8.1 only #7

merged 8 commits into from
Oct 7, 2022

Conversation

guidobonuzzi
Copy link
Contributor

This PR includes:

  • The constructor is now private
  • Static constructor between, after, before, forEmptySequence, fromString
  • Creating a Rank at the beginning or at the end is now more efficient
  • rankValidator with new Exceptions
  • Tests improvement
  • Psalm
  • Coding standard
  • Infection wind Mutation Score Indicator (MSI) of 99%

A new major release is needed, compatibility is broken because the new constructor is private.

…ent, Psalm, Coding Standard, and Infection.

A new major release is needed, compatibility is broken because the new constructor is private.
@Ocramius
Copy link

Ocramius commented Oct 5, 2022

@alexcrawford to be clear, this PR is a proposal for a new major release of this library.

Up to you if you are interested in it: it was done in good faith, to improve the API, stability and behaviour of the library.

I asked @guidobonuzzi to contribute it back to your original project, rather than having us maintain a private fork: hope it is well perceived 👍

Copy link

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM: mostly needs small CI and artifact -related changes

.run/Infection on entire Project.run.xml Outdated Show resolved Hide resolved
infection.json.dist Show resolved Hide resolved
psalm.xml Show resolved Hide resolved
phpcs.xml.dist Show resolved Hide resolved
@alexcrawford
Copy link
Owner

Looks interesting - I will do some integration testing tomorrow.

Is it good practice to commit composer.lock in a project like this?

@guidobonuzzi
Copy link
Contributor Author

Looks interesting - I will do some integration testing tomorrow.

Is it good practice to commit composer.lock in a project like this?

I added composer.lock to .gitignore to prevent commit, but I don't have a sure answer, probably you are right...

@Ocramius, what is the right answer?

@alexcrawford
Copy link
Owner

Ignore my last, I see it's still listed in .gitignore. I should have had my morning coffee before looking at it!

@Ocramius
Copy link

Ocramius commented Oct 6, 2022

FWIW, I endorse committing it, but only in combination with lowest+latest CI pipelines.

The pipeline I proposed @guidobonuzzi to include yesterday has that feature 👍

@Ocramius
Copy link

Ocramius commented Oct 6, 2022

CI won't work here until merged (due to GitHub security model - that's normal).

Meanwhile, here are the CI run results on the fork: https://github.com/KanbanBOX/lexorank-php/actions/runs/3198936467

@guidobonuzzi
Copy link
Contributor Author

CI won't work here until merged (due to GitHub security model - that's normal).

Meanwhile, here are the CI run results on the fork: https://github.com/KanbanBOX/lexorank-php/actions/runs/3198936467

It's impressive how simple it is to implement! Thank you @Ocramius

@Ocramius
Copy link

Ocramius commented Oct 6, 2022

Worked on it for a couple years @_@

composer.json Outdated
@@ -10,7 +10,7 @@
],
"type": "project",
"require": {
"php": ">=8.1",
"php": ">=8.0",
Copy link

Choose a reason for hiding this comment

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

Let's please use ^8.0 - we don't know if PHP 9.0 will be compatible :D

I also endorse ~8.0 || ~8.1, as explained in laminas/laminas-diactoros#117 (comment)

Copy link

Choose a reason for hiding this comment

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

@guidobonuzzi important to fix this previous to merge BTW

Copy link

Choose a reason for hiding this comment

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

Handled in 6a3df83

@alexcrawford alexcrawford merged commit 2762049 into alexcrawford:master Oct 7, 2022
@Ocramius
Copy link

Ocramius commented Oct 7, 2022

That was quick :O

@alexcrawford
Copy link
Owner

Merged and released as 2.0.0

@Ocramius
Copy link

Ocramius commented Oct 7, 2022

Thanks @guidobonuzzi @alexcrawford! 🥳

@guidobonuzzi
Copy link
Contributor Author

Thanks!

@boesing
Copy link

boesing commented Oct 9, 2022

FYI as I stumbled upon this: ~8.0 === ^8.0

@Ocramius
Copy link

Ocramius commented Oct 9, 2022

Argh, indeed @_@

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.

4 participants