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.

Sign up for GitHub

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

Jump to bottom

Support recursive dicts #3

Merged
merged 6 commits into from
Oct 19, 2023
Merged

Support recursive dicts #3

merged 6 commits into from
Oct 19, 2023

Conversation

eak24
Copy link
Contributor

@eak24 eak24 commented Sep 26, 2023

No description provided.

@rkdarst
Copy link
Member

rkdarst commented Sep 27, 2023

This is really fancy! Should it be made even more general, for example jsonpath lookups or something? (I'm not even sure what the right term would be, clearly it's not literal json)

I'm definitely happy for this, just wonder if it should be made even better. Or maybe that's too much change.

@rkdarst
Copy link
Member

rkdarst commented Sep 27, 2023

I've updated the GitHub action if you want to merge/rebase.

@eak24
Copy link
Contributor Author

eak24 commented Sep 27, 2023

I was originally thinking of something like json path lookups - I think that could be useful in the future - for now this is a good stopgap for our use case.

rkdarst
rkdarst reviewed Sep 27, 2023
View reviewed changes
Copy link
Member

@rkdarst rkdarst left a comment

Choose a reason for hiding this comment

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

I like the idea but some comments, especially about the substring matching.

.gitignore Outdated Show resolved Hide resolved
sphinx_ext_substitution/get_replacements.py Outdated Show resolved Hide resolved
sphinx_ext_substitution/substitution.py Outdated Show resolved Hide resolved
@rkdarst
Copy link
Member

rkdarst commented Oct 19, 2023

Duplicate name of the test fixture doc1_original caused problems... took a while to figure out but looks good! Thanks for all your work on this!

@rkdarst rkdarst merged commit 755a2a8 into NordicHPC:master Oct 19, 2023
5 checks passed
@rkdarst
Copy link
Member

rkdarst commented Oct 19, 2023

And I updated the release action and released! Thanks again!

(I commented out the partial_match part in the documentation since it wasn't implemented, but having it be a configurable option is completely reasonable by me if it helps your use cases).

@eak24
Copy link
Contributor Author

eak24 commented Oct 20, 2023

This is great! Thanks for getting it past the finish line!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkdarst rkdarst rkdarst left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eak24 @rkdarst