-
Notifications
You must be signed in to change notification settings - Fork 2
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
Flux commands for analyzing input data #300
Conversation
The first module is ready for functional review, assigning @TobiasNx. Deployed to playground for testing: your sample from #294, using the new From the documentation annotation:
(If we count it's always sorted by highest frequency, because if we count we'll automatically sort internally, which would make it hard to have a consistent implementation with separate control over the sorting. I think this way it's both conceptually simple and hopefully covers your use cases.) |
And the second module, for listing values, is ready for review too. Deployed to playground: the example from above, but calling From the documentation annotation:
(Sorting behavior is as described for |
Implementation looks good. something like this:
|
Also perhaps the template should be |
Deployed to playground for testing:
I've changed both templates to use (It seems I currently have to open these playground links in a new window, otherwise it's empty.) |
Great paths are unique now. +1 Concerning the sorting I thought about it again.
This does not sort by occurence automatically. How does your function deviate from this? The alphabetical sorting helps to get an understanding of the source data and its schema. Therefore I would prefer if we also would support alphabetical sorting too. |
I'm only trying to implement your requirement, I don't have my own preference here. Could you give me an example of the desired output for both commands? |
Just a side note: Wouldn't it make sense to make the template configurable? |
[path] | [occurrence] vs |
but keep it the way you implement it |
I've changed it to sort alphabetically if
I've changed it to use fix-list-paths With an option to set it (thanks for the input @blackwinter): |
Thanks, thats all great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some (more or less) minor remarks.
metafix/src/main/java/org/metafacture/metafix/MetafixListPaths.java
Outdated
Show resolved
Hide resolved
metafix/src/main/java/org/metafacture/metafix/MetafixListValues.java
Outdated
Show resolved
Hide resolved
metafix/src/main/java/org/metafacture/metafix/MetafixListPaths.java
Outdated
Show resolved
Hide resolved
metafix/src/main/java/org/metafacture/metafix/MetafixListValues.java
Outdated
Show resolved
Hide resolved
metafix/src/main/java/org/metafacture/metafix/MetafixListPaths.java
Outdated
Show resolved
Hide resolved
metafix/src/main/java/org/metafacture/metafix/MetafixListPaths.java
Outdated
Show resolved
Hide resolved
metafix/src/main/java/org/metafacture/metafix/MetafixStreamAnalyzer.java
Outdated
Show resolved
Hide resolved
metafix/src/main/java/org/metafacture/metafix/MetafixStreamAnalyzer.java
Outdated
Show resolved
Hide resolved
metafix/src/test/java/org/metafacture/metafix/MetafixListValuesTest.java
Outdated
Show resolved
Hide resolved
- Sort dependencies alphabetically - Rename classes and flux commands for consistency with others - Use backticks for options and their defaults in `@Description` - Remove unused code, add final modifiers See https://gitlab.com/oersi/oersi-etl/-/issues/238
Thank you for the review! Addressed your feedback in 172a701. |
See https://gitlab.com/oersi/oersi-etl/-/issues/238