Skip to content

feat: Implement generators for XML#486

Merged
rholshausen merged 12 commits intopact-foundation:masterfrom
tienvx:implement-xml-generators
May 20, 2025
Merged

feat: Implement generators for XML#486
rholshausen merged 12 commits intopact-foundation:masterfrom
tienvx:implement-xml-generators

Conversation

@tienvx
Copy link
Copy Markdown
Contributor

@tienvx tienvx commented Feb 10, 2025

No description provided.

@tienvx tienvx force-pushed the implement-xml-generators branch from 97bbed5 to f3b146a Compare February 10, 2025 15:57
@tienvx tienvx force-pushed the implement-xml-generators branch from f3b146a to 38667be Compare February 11, 2025 01:09
@tienvx tienvx force-pushed the implement-xml-generators branch 3 times, most recently from 45abe79 to e5e421e Compare March 14, 2025 15:44
@tienvx
Copy link
Copy Markdown
Contributor Author

tienvx commented Mar 16, 2025

This step is failing on multiple checks:

Upload Release Assets
Run svenstaro/upload-release-action@v2
Error: Resource not accessible by integration - https://docs.github.com/rest/releases/releases#create-a-release

I think we should disable this step on pull requests. @rholshausen

@rholshausen
Copy link
Copy Markdown
Contributor

rholshausen commented Mar 16, 2025

I don't know why it is running the release build for your PR, I did not do that.

@rholshausen
Copy link
Copy Markdown
Contributor

The only issue I can see with this, is it walks the entire XML tree for every generator. For large XML documents, this would be inefficient. I think that the generate_values_for_xml_element could return if the current node does not match part of the generator path, or the current recursion depth is greater than the length of the generator path.

I.e. if the generator path is $.parent.child, then when checking the child nodes, no point is checking their children.

Can also optimise for attributes and text nodes. For those, the generator path has to end with either @name or #text. If it doesn't, no need to check those.

@YOU54F
Copy link
Copy Markdown
Member

YOU54F commented Mar 17, 2025

This step is failing on multiple checks:

Upload Release Assets
Run svenstaro/upload-release-action@v2
Error: Resource not accessible by integration - https://docs.github.com/rest/releases/releases#create-a-release

I think we should disable this step on pull requests. @rholshausen

agreed, previously the upload step was only setup to run for release tags

startsWith(github.ref, 'refs/tags/libpact_ffi') ||

although we had some CI optimisations that broke the release workflow, so we did a revert, and I think some things got missed.

It would still be nice to get the ci optimisations in, however scoping the publish assets step to release tags only, would probably be better?

@YOU54F
Copy link
Copy Markdown
Member

YOU54F commented Mar 17, 2025

I don't know why it is running the release build for your PR, I did not do that.

it was setup to smoke test the release build, prior to merging to master, doing the release and finding out it doesn't build. (it the only place we build all the cross platform binaries). it should be scoped to only upload on release though ideally.

@YOU54F
Copy link
Copy Markdown
Member

YOU54F commented Mar 17, 2025

I.e. if the generator path is $.parent.child, then when checking the child nodes, no point is checking their children.

Can also optimise for attributes and text nodes. For those, the generator path has to end with either @name or #text. If it doesn't, no need to check those.

Excellent suggestions!

@tienvx
Copy link
Copy Markdown
Contributor Author

tienvx commented Mar 18, 2025

The release pipeline now also runs on PRs. If being run on PRs, a
non-release build done as it is faster.

I think the Release workflow is run on PRs on purpose. Can you help me fix these failing checks @JP-Ellis ? I think it's related to permission issue.

@tienvx
Copy link
Copy Markdown
Contributor Author

tienvx commented Mar 18, 2025

I refactored the code to implement the suggested performance improvements.

@rholshausen
Copy link
Copy Markdown
Contributor

This PR is still in draft mode?

@tienvx tienvx marked this pull request as ready for review March 25, 2025 05:38
@tienvx
Copy link
Copy Markdown
Contributor Author

tienvx commented Mar 25, 2025

It is ready for review, there are a few notes:

@tienvx tienvx force-pushed the implement-xml-generators branch from 31be46b to ad178bc Compare May 18, 2025 00:54
@tienvx tienvx force-pushed the implement-xml-generators branch from ca24de6 to 339dba6 Compare May 18, 2025 07:21
@tienvx tienvx force-pushed the implement-xml-generators branch from 339dba6 to 141b87d Compare May 18, 2025 10:05
@tienvx
Copy link
Copy Markdown
Contributor Author

tienvx commented May 19, 2025

I fixed the failed checks. Please review again @rholshausen . Be aware of #492

@rholshausen
Copy link
Copy Markdown
Contributor

I know of #492, I just don't know why it happens, but it feels like a typical race condition.

@rholshausen rholshausen merged commit 051ba58 into pact-foundation:master May 20, 2025
36 checks passed
@tienvx tienvx deleted the implement-xml-generators branch May 20, 2025 01:21
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.

3 participants