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 fix method to set variable with element value #282 #365

Merged
merged 7 commits into from
Oct 14, 2024

Conversation

TobiasNx
Copy link
Collaborator

@TobiasNx TobiasNx commented Sep 18, 2024

Resolves #282

@TobiasNx TobiasNx force-pushed the 282-variableFromElementValue branch 2 times, most recently from ef19462 to a05d0e9 Compare September 23, 2024 13:18
@TobiasNx TobiasNx marked this pull request as ready for review September 23, 2024 13:38
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've updated the implementation to account for missing values and added tests for edge cases (a52dc9b).

Open questions:

  1. How to handle missing values? Is an empty string the right choice?
  2. How to handle non-string values? Is the Java object representation the right choice? (Previously, you said that it "should only work with string-elements".)

Finally, the new function needs to be documented.

@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Oct 2, 2024

I've updated the implementation to account for missing values and added tests for edge cases (a52dc9b).

Open questions:

1. How to handle missing values? Is an empty string the right choice?

I would not add a variable if the element does not exist.

2. How to handle non-string values? Is the Java object representation the right choice? (Previously, you [said](https://github.com/metafacture/metafacture-fix/issues/282#issuecomment-2355583039) that it "should only work with string-elements".)

I thought MF only processes string values in the Stream-Processing. Because of that we introduced marker for numbers in the json-encoder. The idea with string was the connection to the other ticket in mf-core but I have put the idea in core on hold.

Finally, the new function needs to be documented.

@blackwinter
Copy link
Member

I thought MF only processes string values in the Stream-Processing.

Metafix knows data types strings, arrays and objects; variables can only contain strings.

@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Oct 2, 2024

I thought MF only processes string values in the Stream-Processing.

Metafix knows data types strings, arrays and objects; variables can only contain strings.

Ah, I forgot. Then I would suggest an exception that the function expected a string but got hash or array.

@blackwinter
Copy link
Member

blackwinter commented Oct 2, 2024

I would not add a variable if the element does not exist.

This might become difficult to use:

  1. If one record has the field and the next one doesn't, the variable keeps its previous value. We might want to remove the variable instead.
  2. Referencing a non-existing variable (if we remove it or if it just hasn't been set yet) results in an error. So you can't reliably use the variable. We might have to introduce a conditional that explicitly checks for the presence of a variable; or add an option to specify the default value.

@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Oct 2, 2024

I would not add a variable if the element does not exist.

This might become difficult to use:

1. If one record has the field and the next one doesn't, the variable keeps its previous value. We might want to remove the variable instead.

2. Referencing a non-existing variable (if we remove it or if it just hasn't been set yet) results in an error. So you can't reliably use the variable. We might have to introduce a conditional that explicitly checks for the presence of a variable; or add an option to specify the default value.

I favor the the option to define a default value. The conditional for the a variable sounds good. I am also in favor to delete the variable at the end of each record.

@blackwinter
Copy link
Member

Then I would suggest an exception that the function expected a string but got hash or array.

Implemented in cee638a.

I favor the the option to define a default value.

Implemented in 46d1ef5.

The conditional for the a variable sounds good. I am also in favor to delete the variable at the end of each record.

Should probably be handled in separate issues.

@TobiasNx
Copy link
Collaborator Author

@TobiasNx I added documentation. Should @fsteeg do a code review?

@TobiasNx TobiasNx assigned blackwinter and unassigned TobiasNx Oct 14, 2024
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@blackwinter blackwinter assigned TobiasNx and unassigned blackwinter Oct 14, 2024
@blackwinter
Copy link
Member

Should @fsteeg do a code review?

Sure, why not.

@TobiasNx TobiasNx assigned fsteeg and unassigned TobiasNx Oct 14, 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.

Very useful feature!

@fsteeg fsteeg assigned TobiasNx and unassigned fsteeg Oct 14, 2024
@TobiasNx TobiasNx merged commit 82d36d3 into master Oct 14, 2024
1 check passed
@TobiasNx TobiasNx deleted the 282-variableFromElementValue branch October 14, 2024 14:52
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.

Create variables from element values or names.
3 participants