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 enumerate feature to convert operator #116

Merged
merged 3 commits into from
Feb 17, 2021
Merged

Conversation

van-fs
Copy link
Member

@van-fs van-fs commented Feb 15, 2021

This PR addresses a few issues seen with Tealium users.

  • The sample rule needs frequent updates to convert strings to numbers
  • order_ information was not included in the original sample
  • The convert operator could do a better job of intelligently converting to numbers

An enumerate option was added to convert. The intent is that if specified, convert will try to automatically convert strings to numbers. There was a fair amount of cleanup needed in convert, and I broke the handler into stages:

  • Enumerate any strings if desired
  • Convert any additional properties specifically
  • Reduce any single item arrays to a single value if needed

Two test utility methods expectValid and expectInvalid were added to make the validate tests more legible. (We should refactor other tests later).

Copy link
Contributor

@TrevorFSmith TrevorFSmith left a comment

Choose a reason for hiding this comment

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

LGTM.

I left a comment asking whether the operator should error or warn when incorrectly configured, but that's nit that you can ignore or address at will.

expectInvalid({ name: 'convert', type: 'real' }, 'must specify properties');
expectInvalid({ name: 'convert', enumerate: 'true' }, 'enumerate should be a bool');
expectInvalid({ name: 'convert', properties: ['price', 'tax'], type: 'array' }, 'array is not valid');
expectInvalid({
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there conflicting combinations of properties that we should error or warn about?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I have them all accounted for. The one gray area is probably force with enumerate. I'll add a card to track.

@TrevorFSmith TrevorFSmith assigned van-fs and unassigned TrevorFSmith Feb 17, 2021
@van-fs
Copy link
Member Author

van-fs commented Feb 17, 2021

Validation of rules results in throwing Error, which in the Observer gets handled as logger.error. I've started to use warn more so as an indicator that something unexpected occurred but a total failure did not occur. For example, failing to register a rule is an error. Being unable to convert a property is a warning because as messy as data layers are, it's possible to convert successfully on one page but incorrectly on another. I think it's helpful to dial back this level of feedback so that errors are surfacing more widespread, total failures while warnings could be sporadic or partial failures.

@van-fs van-fs merged commit edfd3a2 into main Feb 17, 2021
@van-fs van-fs deleted the van/convert-enumerate branch February 17, 2021 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants