Skip to content
This repository has been archived by the owner on Jan 27, 2025. It is now read-only.

Add print_record() Fix function. #238

Merged
merged 6 commits into from
Jul 8, 2022
Merged

Conversation

blackwinter
Copy link
Member

Allows to print the current record during transformation (even at different stages). In Limetrans we implemented it (in a simplified form) as a custom Fix function (cf. #105), but maybe it's useful generally.

Tests are still missing as I'm unsure how to go about it. Any suggestions?

@TobiasNx: Do you also want to do a functional review?

README.md Outdated
@@ -242,6 +242,37 @@ Copies (or appends to) a field from an existing field.
copy_field("<sourceField>", "<targetField>")
```

##### `debug_record`

Prints the current record either to standard output or to a file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Prints the current record either to standard output or to a file.
Prints the current record at the position of the function either to standard output or to a file. It can help to debug the fix-transformation.

@TobiasNx
Copy link
Collaborator

TobiasNx commented Jul 5, 2022

This seems to be a nice feature.
Had a look at the documentation but did not try it yet.
I think I could try it in context of ALMA-FIX. I then could try it in the transformation

+1 for merging.

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.

Looks good and useful! Two things I noticed:

  • I think I'd prefer print_record as the name, since that's what it actually does, while debugging is basically what we use the printed output for. I think that would make clearer what the function does.
  • Considering the fact that JSON output is off by default, we add a lot of code for that. Maybe it would make sense to have that on by default?

About the testing issue you mentioned, couldn't we print to a temp file (File.createTempFile) and compare that with some expected result (from a string variable in the unit test)?

@blackwinter
Copy link
Member Author

I think I'd prefer print_record as the name, since that's what it actually does, while debugging is basically what we use the printed output for. I think that would make clearer what the function does.

Yes, I was emphasizing the intended purpose, so it's clearer what you'd use it for. But I'm fine either way. I'll change the name.

Considering the fact that JSON output is off by default, we add a lot of code for that. Maybe it would make sense to have that on by default?

As you maybe noticed, I only added the JSON output here, we didn't have that before in Limetrans. Actually, I'd be fine with having it as the sole output format (the default output is only really useful for display purposes anyway, you can't really do anything with it).

(Regardless of that decision, the JSON encoding will be reused for the to_json() function which I'm working on at the moment; mostly because it comes in a pair with from_json() which we might have a need for.)

About the testing issue you mentioned, couldn't we print to a temp file (File.createTempFile) and compare that with some expected result (from a string variable in the unit test)?

Yes, I'll come up with something for to_json() and then apply it here as well. In the worst (simplest) case it'll just be a plain string comparison, no diff or anything sophisticated.

@blackwinter
Copy link
Member Author

Also, I think I'd like to change prefix to a format string just like destination. So you can choose to have the ID included or not.

@fsteeg
Copy link
Member

fsteeg commented Jul 7, 2022

Actually, I'd be fine with having it [JSON] as the sole output format

+1

Also, I think I'd like to change prefix to a format string just like destination

+1

@fsteeg fsteeg assigned blackwinter and unassigned fsteeg Jul 7, 2022
@blackwinter blackwinter changed the title Add debug_record() Fix function. Add print_record() Fix function. Jul 7, 2022
@blackwinter blackwinter merged commit bec6763 into master Jul 8, 2022
@blackwinter blackwinter deleted the addDebugRecordFunction branch July 8, 2022 12:00
blackwinter added a commit that referenced this pull request Jul 15, 2022
…l representation. (#238, 74dc79f)

This is the only way to see the record without interference from the encoding process.
@blackwinter
Copy link
Member Author

I've added the output of the internal representation back (be32875) as it's the only way to see the record without interference from the encoding process (e.g. in the context of investigating this error).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants