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

Be more strict with Composer version constraints #756

Merged
merged 71 commits into from
Jul 9, 2023

Conversation

fredden
Copy link
Contributor

@fredden fredden commented Nov 10, 2022

This pull request changes the Composer version constraint normalisation to be more opinionated / strict.

Please let me know if you would prefer to have these enhancements as individual / separate pull requests. It may be that some of these rules are desired whereas others are too subjective / opinionated for now.

Changes include:

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Merging #756 (f6f7616) into main (5c1172c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head f6f7616 differs from pull request most recent head 8631af0. Consider uploading reports for the commit 8631af0 to get more accurate results

@@             Coverage Diff              @@
##               main     #756      +/-   ##
============================================
+ Coverage     97.40%   97.44%   +0.04%     
- Complexity      162      166       +4     
============================================
  Files            30       30              
  Lines           693      705      +12     
============================================
+ Hits            675      687      +12     
  Misses           18       18              
Impacted Files Coverage Δ
...rc/Vendor/Composer/VersionConstraintNormalizer.php 97.82% <100.00%> (+0.20%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@localheinz localheinz self-assigned this Nov 10, 2022
@fredden
Copy link
Contributor Author

fredden commented Nov 21, 2022

@localheinz is there anything I can do to help get this merged in? I'm happy to split this into individual pull requests if you'd prefer to evaluate each item separately.

@localheinz
Copy link
Member

@fredden

Apologies for the delay, I am slowly catching up with open issues in the @ergebnis organization!

@fredden
Copy link
Contributor Author

fredden commented Nov 28, 2022

I've resolved the merge conflict with a merge commit. Please let me know if you'd prefer a re-base.

@localheinz
Copy link
Member

@fredden

Since I made a bunch of changes in main, and do not want to put more burden on you, I will be happy to take over your pull request - is that ok with you?

@fredden
Copy link
Contributor Author

fredden commented Jan 1, 2023

Yes, that's fine with me. I'm happy to update this pull request, but probably won't have capacity to do so for a few days at least.

@fredden
Copy link
Contributor Author

fredden commented Jan 2, 2023

@localheinz I've merged in the changes from main and fixed the tests to expect the new behaviour. However the mutation score index has gone down a lot and is now below the threshold. (It wasn't before.) I suspect this means we need more tests, but the the new test fixtures format / layout is so different now. Please can you have a look at this aspect?

Also, I noticed that the sentences used for version constraints are now all messed up. I decided to leave this as-is, as these are nonsense version constraints, so having nonsense come out seems fine to me. For example,

"ergebnis/php-cs-fixer-config": "Provides a and configuration factory for friendsofphp/php-cs-fixer. multiple rule sets",

@localheinz
Copy link
Member

@fredden

I like this change, but I am going to prepare a release for 4.0.0 without this improvement - we can follow up!

@VincentLanglet
Copy link

Next, we need decisions made about the remaining suggestions:

  • Prefer wildcard (1.2.*) or tilde operator (~1.2.0) when equivalent, or leave as-is.

  • Prefer caret (^2.4) or tilde (~2.4) operator when equivalent, or leave as-is.

  • Assert min/max number of 'parts' when equivalent (^1, ^1.0, ^1.0.0), or leave as-is.

Since it might be subject to debate, what about using configuration for this ?

Something like "preferred operator" or "min part number" ?

@fredden
Copy link
Contributor Author

fredden commented Jul 3, 2023

@localheinz this pull request came up in discussion with a colleague today. Is there anything I can do to help progress this?

@localheinz
Copy link
Member

@fredden

I am removing the adjustments for parts (for now), because I am a bit unsure whether people will like it or not.

I certainly would, but I am in favour of three instead of two parts, so major.minor.patch instead of major.minor.

@localheinz localheinz force-pushed the version-normalisation branch from f6f7616 to 8631af0 Compare July 9, 2023 15:52
Copy link
Member

@localheinz localheinz left a comment

Choose a reason for hiding this comment

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

👍

@localheinz localheinz merged commit 1550e5e into ergebnis:main Jul 9, 2023
@localheinz
Copy link
Member

Thank you, @fredden!

@fredden fredden deleted the version-normalisation branch July 9, 2023 16:29
@fredden
Copy link
Contributor Author

fredden commented Jul 9, 2023

Thanks very much for considering these features. I've seen benefit from those that were released as part of v4.0.0 already, and I look forward to those that have been released now too. I'll leave the idea about min/max 'parts' with you; let me know if you'd like me to open a pull request for that feature.

@localheinz
Copy link
Member

Thank you very much, @fredden - I still have a backup of your branch, so I will open a pull request soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants