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

Default behaviour of copy_field/move_field/add_field should be destructive, the appending behaviour creates problems #116

Closed
3 tasks done
TobiasNx opened this issue Jan 28, 2022 · 11 comments · Fixed by #313
Closed
3 tasks done

Default behaviour of copy_field/move_field/add_field should be destructive, the appending behaviour creates problems #116

TobiasNx opened this issue Jan 28, 2022 · 11 comments · Fixed by #313
Assignees
@blackwinter
Labels
bug Something isn't working conformToCatmandu RevistAfter102
Projects
Fix & Playground Public Beta

Comments

@TobiasNx
Copy link
Collaborator

TobiasNx commented Jan 28, 2022 •
edited by blackwinter
Loading

See here
(The error seems to be not virulent any more, but the underlying behaviour is still productive.)

Catmandu Fix default behaviour is destructive

Appending should only be done explicitly and copy/move/add_field should be destructive.
Could be that this is the root of #113
@TobiasNx TobiasNx added the bug Something isn't working label Jan 28, 2022
@TobiasNx TobiasNx changed the title Non-destructive behaviour of copy/move/add_field thorugh appending causes problems Default non-destructive behaviour of copy/move/add_field through appending causes problems Jan 28, 2022
blackwinter added a commit to hbz/Catmandu that referenced this issue Feb 3, 2022
@blackwinter
copy_field/move_field/add_field testing (metafacture/metafacture-fix#116
4d2f0e3 
)
@blackwinter
Copy link
Member

Catmandu is destructive: hbz/Catmandu@4d2f0e3.

@fsteeg
Copy link
Member

fsteeg commented Feb 3, 2022 •
edited by blackwinter
Loading

To avoid conflict with #102, should be started after the planned first PR for #102.

Functional review: @TobiasNx
Code review: @fsteeg

This was referenced Feb 3, 2022
Closed
Closed
@TobiasNx TobiasNx changed the title Default non-destructive behaviour of copy/move/add_field through appending causes problems Default non-destructive behaviour of copy_field/move_field/add_field through appending causes problems Feb 8, 2022
@TobiasNx TobiasNx added the needsIntegrationTest Integration test(s) should be provided. label Feb 28, 2022
TobiasNx added a commit that referenced this issue Mar 1, 2022
@TobiasNx
Add integration test for #116
7958d80
@TobiasNx TobiasNx mentioned this issue Mar 1, 2022
Merged
@TobiasNx TobiasNx removed the needsIntegrationTest Integration test(s) should be provided. label Mar 1, 2022
TobiasNx added a commit that referenced this issue Mar 2, 2022
@TobiasNx
Merge pull request #183 from metafacture/160-integrationTestsFor116
c94a9ba 
Add integration test for #116
@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Jun 13, 2022 •
edited
Loading

Since this is standard behaviour of Catmandu fix I would like when this gets prioritized over other fix tickets.

@TobiasNx TobiasNx added help wanted Extra attention is needed RevistAfter102 and removed help wanted Extra attention is needed labels Jun 20, 2022
@TobiasNx TobiasNx mentioned this issue Jul 5, 2022
Open
@TobiasNx TobiasNx changed the title Default non-destructive behaviour of copy_field/move_field/add_field through appending causes problems Default behaviour of copy_field/move_field/add_field should be destructive, the appending behaviour creates problems Apr 13, 2023
blackwinter added a commit that referenced this issue Jun 6, 2023
@blackwinter
Change add_field() Fix function to destructive behaviour for Catman…
5bc0378 
…du compatibility. (#116)

NOTE: `add_field()` and `set_field()` are now equivalent, but according to Catmandu's documentation `set_field()` "will not create the intermediate structures if they are missing".
@blackwinter blackwinter mentioned this issue Jun 6, 2023
Merged
@blackwinter
Copy link
Member

@TobiasNx: add_field() behaviour is going to be changed with #308. Please test it.

@blackwinter blackwinter assigned TobiasNx and unassigned blackwinter Jun 6, 2023
blackwinter added a commit to hbz/limetrans that referenced this issue Jun 6, 2023
@blackwinter
Prepare for Metafix destructive add_field(). (metafacture/metafactu…
cc766f4 
…re-fix#116)
blackwinter added a commit to hbz/limetrans that referenced this issue Jun 7, 2023
@blackwinter
Prepare for Metafix destructive add_field(). (metafacture/metafactu…
03fabe4 
…re-fix#116)
@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Jun 9, 2023

@TobiasNx: add_field() behaviour is going to be changed with #308. Please test it.

@blackwinter tested it. seems to work :) +1

@TobiasNx TobiasNx assigned blackwinter and unassigned TobiasNx Jun 9, 2023
blackwinter added a commit to hbz/limetrans that referenced this issue Jun 12, 2023
@blackwinter
Prepare for Metafix destructive add_field(). (metafacture/metafactu…
f992bb4 
…re-fix#116)
blackwinter added a commit that referenced this issue Jun 13, 2023
@blackwinter
Change copy_field()/move_field() Fix functions to destructive beh…
2427f2c 
…aviour for Catmandu compatibility. (#116)

`move_field()` is implemented in terms of `copy_field()`, so changing the latter's behaviour also changes the former's.
blackwinter added a commit to hbz/limetrans that referenced this issue Jun 13, 2023
@blackwinter
Prepare for Metafix destructive copy_field()/move_field(). (metaf…
c7b7dfa 
…acture/metafacture-fix#116)
@blackwinter blackwinter mentioned this issue Jun 13, 2023
Merged
@blackwinter
Copy link
Member

@TobiasNx: copy_field()/move_field() behaviour is going to be changed with #313. Please test it.

@blackwinter blackwinter assigned TobiasNx and unassigned blackwinter Jun 13, 2023
@TobiasNx
Copy link
Collaborator Author

+1 tested different variations. Thanks a lot!

@TobiasNx TobiasNx assigned blackwinter and unassigned TobiasNx Jun 15, 2023
@blackwinter
Copy link
Member

blackwinter commented Jun 15, 2023 •
edited
Loading

tested different variations.

Thanks, but just to be sure: Have you also verified that your current transformations (lobid-resources, oersi, etc.) are going to work with these changes? If not, do you maybe want to do it before we merge?

(The required changes in Limetrans were quite extensive: hbz/limetrans@f992bb4 & hbz/limetrans@c7b7dfa)

@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Jun 15, 2023 •
edited
Loading

tested different variations.

Thanks, but just to be sure: Have you also verified that your current transformations (lobid-resources, oersi, etc.) are going to work with these changes? If not, do you maybe want to do it before we merge?

(The required changes in Limetrans were quite extensive: hbz/limetrans@f992bb4 & hbz/limetrans@c7b7dfa)

I did not test this before but now i did. But I always assumed the destructiv behaviour to be valid and wrote my scripts accordingly. lobid resources seems to work well. oersi too.

@TobiasNx
Copy link
Collaborator Author

(I updated my comment, I did test it. :) )

@TobiasNx
Copy link
Collaborator Author

+1

blackwinter added a commit to hbz/limetrans that referenced this issue Jun 16, 2023
@blackwinter
Prepare for Metafix destructive add_field(). (metafacture/metafactu…
1b52f35 
…re-fix#116)
blackwinter added a commit to hbz/limetrans that referenced this issue Jun 16, 2023
@blackwinter
Prepare for Metafix destructive copy_field()/move_field(). (metaf…
10c4250 
…acture/metafacture-fix#116)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@blackwinter blackwinter

Labels
bug Something isn't working conformToCatmandu RevistAfter102
Projects
Metafacture
Archived in project
Fix & Playground Public Beta
Ready
Milestone
No milestone
3 participants
@blackwinter @fsteeg @TobiasNx