Skip to content

Conversation

@szepeviktor
Copy link
Contributor

Specifying the version yourself will most likely end up creating problems

https://getcomposer.org/doc/04-schema.md#version

@szepeviktor
Copy link
Contributor Author

Copy link
Member

@AlessandroMinoccheri AlessandroMinoccheri left a comment

Choose a reason for hiding this comment

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

Thanks @szepeviktor for this PR, but if you would like to remove the version from the composer.json file, we should provide a way to print the version in a testable way.
And as you can see, VersionTest is broken.

@szepeviktor
Copy link
Contributor Author

How about adding version as constant here?

class Version

@fain182
Copy link
Collaborator

fain182 commented Mar 15, 2025

I have read the links above, but if the risk would be that of " Specifying the version yourself will most likely end up creating problems at some point due to human error." then I don't see any advantage in writing the version in one place or another.

If we find a way to avoid writing the version in the repository, so we don't have to update it manually with every release, then that’s a different matter.

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Mar 15, 2025

While testing it is not possible to get the future version number. (only existing git tags can be queried)

While running:

Composer\InstalledVersions::getVersion('phparkitect/arkitect');

@micheleorselli
Copy link
Member

While testing it is not possible to get the future version number. (only existing git tags can be queried)

While running:

Composer\InstalledVersions::getVersion('phparkitect/arkitect');

That should do the trick, we could use it in the Version class instead of parsing the composer.json., defaultingo to UNKNOWN if the class InstalledVersions does not exists. BTW I guess the right package name is

Composer\InstalledVersions::getVersion('phparkitect/phparkitect');

@szepeviktor
Copy link
Contributor Author

Google Gemini talks about these: https://g.co/gemini/share/dc76096d2ddc

@AlessandroMinoccheri
Copy link
Member

@szepeviktor do you wanna try to implement it :) ?

@szepeviktor
Copy link
Contributor Author

@szepeviktor
Copy link
Contributor Author

It must be more complicated than I thought ...

--- Expected
+++ Actual
@@ @@
-'0.5.2'
+'dev-f3390ad0a999714bcc718754ad301fd8d8819716'

$this->assertEquals('0.5.2', Version::get());

@micheleorselli
Copy link
Member

It must be more complicated than I thought ...

--- Expected
+++ Actual
@@ @@
-'0.5.2'
+'dev-f3390ad0a999714bcc718754ad301fd8d8819716'

$this->assertEquals('0.5.2', Version::get());

Considering that with this change everything is managed via composer, does this test still provide value? I'm for removing it

@szepeviktor
Copy link
Contributor Author

Done 🍏

@codecov
Copy link

codecov bot commented Mar 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.94%. Comparing base (3acff7c) to head (23eba2d).
Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #462      +/-   ##
============================================
+ Coverage     94.90%   94.94%   +0.04%     
+ Complexity      605      604       -1     
============================================
  Files            69       69              
  Lines          1609     1603       -6     
============================================
- Hits           1527     1522       -5     
+ Misses           82       81       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@micheleorselli micheleorselli merged commit ed2e861 into phparkitect:main Mar 24, 2025
18 checks passed
@szepeviktor
Copy link
Contributor Author

Glad to contribute.

@szepeviktor szepeviktor deleted the patch-1 branch March 24, 2025 22:23
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