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

Add new functions add_array and add_hash #374 #379

Merged
merged 10 commits into from
Nov 7, 2024

Conversation

TobiasNx
Copy link
Collaborator

Resolves #374

This is a preparation for #309 which will change the behavior of all set-functions as set_array and set_hash which will not create the intermediate structure of a path.

Since we rely in context of oersi and lobid on this specific functionality I introduced add_array and add_hash which copy the current implementation of set_array and set_hash.

I also changed the tests from set_array and set_hash to add_array and add_hash since #307 will need new integration tests and java tests.

The are a copy of the current implementation of set_array and set_hash. This is a preparation for #309
As a preliminary work for #309 since set_array will change its behaviour but add_array will keep the functionality.
As a preliminary work for #309 since set_array will change its behaviour but add_array will keep the functionality.
…374

Additional preliminary work for #309 when the set-fixes change their functionality.
@TobiasNx TobiasNx changed the title Add new function add_array and add_hash #374 Add new functions add_array and add_hash #374 Oct 16, 2024
Copy link
Member

@blackwinter blackwinter left a comment

Choose a reason for hiding this comment

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

I would suggest to change all set_* implementations to delegate to the corresponding add_* functions, so we don't have to maintain two identical implementations for each pair until the former have been replaced.

I would also argue that we should keep set_* where appropriate, because these are the "standard" functions; the new functions aren't supported by Catmandu (yet).

Similarly, all the tests should be left untouched in order to surface any actual behaviour changes once the set_* implementations are changed. Just add new ones where appropriate.

@blackwinter blackwinter removed their assignment Oct 17, 2024
Copy link
Member

@fsteeg fsteeg left a comment

Choose a reason for hiding this comment

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

Agree about suggestions by @blackwinter.

@fsteeg fsteeg assigned TobiasNx and unassigned fsteeg Oct 18, 2024
@TobiasNx
Copy link
Collaborator Author

@blackwinter like this?

Copy link
Member

@blackwinter blackwinter left a comment

Choose a reason for hiding this comment

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

Yes, thanks!

The new functions need to be documented, though.

As for the tests, I assume that we will want to introduce them once the behaviour gets changed (switch existing failing tests to add_*, add matching new tests for set_*).

@blackwinter blackwinter assigned TobiasNx and unassigned blackwinter Nov 6, 2024
@TobiasNx TobiasNx assigned fsteeg and unassigned TobiasNx Nov 7, 2024
@fsteeg fsteeg assigned TobiasNx and unassigned fsteeg Nov 7, 2024
@TobiasNx TobiasNx merged commit 3bb2b28 into master Nov 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Introduce add_hash and add_array fixes
3 participants