Skip to content

filter: add SetStringFromURL filter#28

Merged
arl merged 4 commits intomasterfrom
add-filter/set-string-from-url
Aug 13, 2020
Merged

filter: add SetStringFromURL filter#28
arl merged 4 commits intomasterfrom
add-filter/set-string-from-url

Conversation

@arl
Copy link
Copy Markdown
Collaborator

@arl arl commented Aug 12, 2020

❓ What

This filter is needed for the FeedParser topology, but making it more generic has no cost, so we end up with a filter that looks for some user-defined strings in a record metadata URL key, such as records read with the Files input. If one of the user-defined
strings is found in the URL, this new filter will sets an user-defined field to that string. Records for which the URL doesn't contain any of the strings are discarded.

🔨 How to test

  1. List all steps necessary;
  2. To test this pull request.

✅ Checklists

This section contains a list of checklists for common uses, please delete the checklists that are useless for your current use case (or add another checklist if your use case isn't covered yet).

  • Is there unit/integration test coverage for all new and/or changed functionality added in this PR?
  • Have the changes in this PR been functionally tested?
  • Has make gofmt-write been run on the code?
  • Has make govet been run on the code? Has the code been fixed accordingly to the output?
  • Have the changes been added to the CHANGELOG.md file?
  • Have the steps in CONTRIBUTING.md been followed to update a Go module?

This filter is needed for the FeedParser topology, but making it
more generic has no cost, so we end up with a filter that looks
for some user-defined strings in a record metadata URL key, such
as records read with the Files input. If one of the user-defined
strings is found in the URL, this new filter will sets an
user-defined field to that string. Records for which the URL
doesn't contain any of the strings are discarded.
@arl arl requested a review from tommyblue August 12, 2020 17:02
Strings []string `help:"Strings to look for in the URL. Discard records not containing any of them." required:"true"`
}

func (cfg *setStringFromURLConfig) fillDefaults() error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe in another PR we could make a general refactoring renaming these fillDefaults() to checkConfsAndFillDefaults() as this is what we do (not only here, everywhere)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, good point. I'd like us to discuss a way to enforce that pattern in all components, maye by making the config interfaces actual type Config interface{ Validate() error } or something, rather than mere interface{}.
Id also like us to think about going further and find an easy way to do that as well as providing an easy way to document required field and default fields but also make applying the defaults, validating the values and providing help all at once.

Copy link
Copy Markdown
Contributor

@tommyblue tommyblue left a comment

Choose a reason for hiding this comment

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

Nor approving neither rejecting because I don't know if the multiple calls to next is what you want. If it is, then it's approved.
If it's not (i.e. the next must be moved after the for loop, then the l.Set() call is wrong because it will overwrite the previous loop value

Comment thread filter/set_string_from_url.go Outdated
Comment thread filter/set_string_from_url_test.go
Comment thread filter/set_string_from_url.go
Comment thread filter/set_string_from_url.go
@arl arl merged commit 112f6e8 into master Aug 13, 2020
@arl arl deleted the add-filter/set-string-from-url branch August 13, 2020 10:16
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