Skip to content

Conversation

@alexandre-daubois
Copy link
Member

Q A
Branch? 7.3
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

I'd suggest considering this one as a bug fix so all supported branches benefit from this.

@nicolas-grekas
Copy link
Member

instead of gitmodules, can't we use composer? I think we do so already for the intl component (or emoji? I don't remember)

@stof
Copy link
Member

stof commented Dec 12, 2025

We could indeed use a package repository for composer. This is what I do for the sass-spec repo in https://github.com/scssphp/scssphp/ (and the setup could be simplified to avoid duplicating the commit reference btw not yet actually: composer/composer#12678).

@nicolas-grekas
Copy link
Member

I was thinking about the composer.json in this directory:
https://github.com/symfony/symfony/tree/8.1/src/Symfony/Component/Emoji/Resources/bin

@alexandre-daubois alexandre-daubois force-pushed the jsonpath-cts-submodule branch 2 times, most recently from f06914a to 49f764e Compare December 15, 2025 15:23
@alexandre-daubois
Copy link
Member Author

Thanks for the hints, I followed and implemented the Emoji component approach

@stof
Copy link
Member

stof commented Dec 15, 2025

To me, the big difference between the way the Emoji component works and the way this PR works is that the Emoji composer-based task is about installing things needed for specific tasks updating committed data files that are part of the component. Running the testsuite does not require running those commands.

In the case of the JsonPath testsuite (or the Yaml testsuite), the testsuite itself is a dev dependency.
I would put that composer package repository in the normal composer.json to avoid extra work for contributors.

@alexandre-daubois
Copy link
Member Author

Proposed a new approach in the last commit taking into account your comment @stof. Updating the test suite would need to edit two files, but I think it's worth it given how easy it makes it to install the suite.

I'm not a big fan of the path guessing in the test case depending on if you use the monorepo or the standalone component, but I'm not sure there's a better solution here?

@alexandre-daubois alexandre-daubois force-pushed the jsonpath-cts-submodule branch 2 times, most recently from c7fc77c to b96ee82 Compare December 17, 2025 11:08
"source": {
"type": "git",
"url": "https://github.com/jsonpath-standard/jsonpath-compliance-test-suite.git",
"reference": "b9d7153e58711ad38bb8e35ece69c13f4b2f7d63"
Copy link
Member

Choose a reason for hiding this comment

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

what about referencing a branch? is that possible? so that we won't have to change this file to get updates?
same below

Copy link
Member Author

@alexandre-daubois alexandre-daubois Dec 17, 2025

Choose a reason for hiding this comment

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

It is possible but explicitly discouraged by Composer (first note block of https://getcomposer.org/doc/05-repositories.md#package-2). Also, the version field is mandatory so this would require to put some placeholder (e.g. 1.0.0) that would be completely independent from the distant repository (which don't provide tags). These are quite a few reasons to stick with the current approach even if it won't update automatically, what do you think?

Copy link
Member

@stof stof Dec 17, 2025

Choose a reason for hiding this comment

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

@nicolas-grekas this won't work fine:

  • if you already have it locally, it will never be updated unless you delete your vendor folder
  • our testsuite won't be reproducible anymore (while we have a setup to skip some tests that are expected to not be passing yet)

Copy link
Member

Choose a reason for hiding this comment

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

then we need some documentation about the process.

@alexandre-daubois alexandre-daubois changed the title [JsonPath] Use git submodules for JsonPath compliance test suite [JsonPath] Use composer packages for JsonPath compliance test suite Dec 17, 2025
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.

4 participants