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

feature/RowProcessor-replaceNewlinesWithSpaces #137

Merged

Conversation

nezda
Copy link
Contributor

@nezda nezda commented Apr 27, 2021

Description

Add optional boolean flag RowProcessor.replaceNewlinesWithSpaces that controls whether or not to replace newlines with spaces in values before passing them to field processors (don't change default behavior by defaulting this to true).

Motivation

TextFieldProcessors may wish to be aware of newlines in the input and encode them as features. Example encoded as a unit test.

@nezda nezda force-pushed the feature/RowProcessor-replaceNewlinesWithSpaces branch 3 times, most recently from cac2bd6 to d8347af Compare April 27, 2021 17:26
@Craigacp Craigacp added the OCA signed This PR is from a person/organisation who has signed the OCA label Apr 27, 2021
@nezda nezda force-pushed the feature/RowProcessor-replaceNewlinesWithSpaces branch from 4175623 to 00c24bf Compare April 28, 2021 17:20
Copy link
Member

@JackSullivan JackSullivan left a comment

Choose a reason for hiding this comment

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

This looks good to me. I originally wrote this, and revisiting it now, I'm not convinced that the default behavior is correct. That is, we should not be altering the incoming data in RowProcessor before it is fed to the feature processors. In a future version we'll probably want to change that, but to avoid breaking changes this is probably the best interim solution. The constructor situation on RowProcessor is also in great need of harmonization, probably through consolidation into a builder pattern.

Thanks for putting in the work to put it in, and for bringing this code to our attention again.

@Craigacp Craigacp merged commit fdf0ef4 into oracle:main Apr 29, 2021
@Craigacp
Copy link
Member

Thanks. As Jack said, we probably want to revisit this as part of a larger refactor to make RowProcessor a little simpler to use, but in the meantime this seems like a fine change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA signed This PR is from a person/organisation who has signed the OCA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants