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 datatable/java #291

Merged
merged 33 commits into from
Apr 7, 2018
Merged

add datatable/java #291

merged 33 commits into from
Apr 7, 2018

Conversation

mpkorstanje
Copy link
Contributor

@mpkorstanje mpkorstanje commented Oct 21, 2017

DataTable is a simple data structure that allows the use and transformation of Gherkin data tables in cucumber without directly using the table from Gherkin.

It also features a type registry similar to that of cucumber expressions to assist the transformation of a data table into other types and data structure. I believe that it currently can handle the transformation of a data table to any reasonable combination of lists, lists of lists, lists of maps, maps, maps of lists and maps of maps.

It does not (yet) feature the automagic conversion currently present in Cucumber JVM. This is a feature that may be added at a later date.

For more usage see the datatable/README.md and datatable/java/README.md

This PR is needed to support the implementation of Cucumber Expressions in Cucumber JVM. As such it also includes several fixes for cucumber-expressions.

@@ -68,21 +68,16 @@ private String buildCaptureRegexp(List<String> regexps) {
}

@Override
public List<Argument<?>> match(String text, final DataTable argument) {
public List<Argument<?>> match(String text, List<List<String>> tableArgument) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the to match(String text, List<List<String>> tableArgument) so callers don't have to provide their own data table with converter which needs a parameter type registry instance. This simplifies the logistics of using Expressions.

public Expression createExpression(String expressionString, final Type tableType) {
if (tableType == null) throw new CucumberExpressionException("tableType can not be null");

return createExpression(expressionString, new Transformer<List<List<String>>, Object>() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can extract the Transformer and leave its creation to the caller. If we also provide the default table transform we can cut the dependency between datatable and cucumber-expressions.

@mpkorstanje mpkorstanje changed the title [WIP] Add datatable/java [wip] add datatable/java Oct 21, 2017
@mpkorstanje
Copy link
Contributor Author

mpkorstanje commented Oct 21, 2017

It's potentially 3 libs, but it makes sense to spike it all in the same project before extracting.

I've opted to use composition instead of extension. Javas type system isn't quite powerful enough to create a sufficiently generic parameter type. So there are now two separate type registries and a third one to tie them together. This makes things allot simpler.

We now have a concept of a StepExpression in cucumber-jvm which is a CucumberExpression + an optional tableOrDocStringType.

Map<K, V> map = new LinkedHashMap<>();
int i = 0;
for (String cell : valueRow) {
map.put(keys.get(i), valueConverter.transform(singletonList(cell)));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is wrong. We should use the match method first and pass that result list to the value converter.

@mpkorstanje
Copy link
Contributor Author

I've come to the conclusion that adding a default object mapper will run us into the same problems and complexity as we've had with XStream. It binds both cucumber and and its user to a specific mapper and requires that our users follow the conventions and ceremonies of that mapper. This makes it harder to use business objects objects in cucumber.

On other other hand by moving the object mapper into the parameter type configuration the end user can decide which mapper to use and use one that works in conjunction with his own business objects. So if the user is using jackson in his domain he can use jackson in cucumber. If the user is using something else, he can use that.

This adds a little verbosity but with java 8 it already folds up nicely:

        parameterTypeRegistry.defineDataTableType(DataTableType.tableOf("groceries", ShoppingStepdefs.Grocery.class, new TableRowTransformer<ShoppingStepdefs.Grocery>() {
            @Override
            public ShoppingStepdefs.Grocery transform(Map<String, String> row) {
                return new ObjectMapper().convertValue(row, ShoppingStepdefs.Grocery.class);
            }
        }));

@mpkorstanje mpkorstanje force-pushed the mpkorstanje-data-table-spike branch 3 times, most recently from 575461e to e248260 Compare January 7, 2018 02:27
@mpkorstanje
Copy link
Contributor Author

mpkorstanje commented Jan 7, 2018

To make development a bit easier I've moved the datatable code into the cucumber-expressions branch of cucumber-jvm.

In hindsight the datatable to list objects scenario is a bit silly. It has the same type as toLists so it would be impossible to disambiguate.

@aslakhellesoy
Copy link
Contributor

I want to have a cross-platform implementation of data tables, just like cucumber-expressions and tag-expressions. Part of that is a shared test suite and synchronised releases.

This becomes harder if it’s moved to
Cucumber-Jvm.

What became easier by moving it?

@mpkorstanje
Copy link
Contributor Author

mpkorstanje commented Jan 8, 2018

Refactoring and integrating with the existing code. IDEA can't refactor over multiple projects.

As I hadn't quite untangled the DataTable from the StepExpression from the StepDefinition yet I ended up in a cycle of refactor-compile-fix-the-other-project. By putting the data tables and step expression into cucumber-jvm the tooling works and it the refactorings happen instantly.

Once cucumber expressions is done but before it is merged I reckon we can extract data tables out again.

@mpkorstanje mpkorstanje force-pushed the mpkorstanje-data-table-spike branch 8 times, most recently from a99516a to 512e328 Compare January 12, 2018 22:33
@mpkorstanje
Copy link
Contributor Author

@aslakhellesoy I've put the code is back in. The good parts to review are README.md, DataTable.java and DataTableTypeRegistryTableConverter. The other code came from cucumber-jvm and has only been lightly touched. I assume you're familiar with it.

Adding extra constructors to ParameterType allows users
to directly define a transform without wrapping it in a
single transformer.

Allowing checked exceptions to be thrown while transforming
parameters reduces the complexity of the transform.

Removed unused parameters from the factory
@mpkorstanje mpkorstanje changed the title [wip] add datatable/java add datatable/java Mar 10, 2018
@aslakhellesoy aslakhellesoy merged commit 9868f86 into master Apr 7, 2018
@mpkorstanje mpkorstanje deleted the mpkorstanje-data-table-spike branch April 8, 2018 08:10
mpkorstanje added a commit to cucumber/cucumber-jvm that referenced this pull request May 14, 2018
 * Implements Cucumber expressions
 * Removes XStream
 * Adds DataTables from cucumber/common#291
 * Introduces the concept of StepExpressions which combines a CucumberExpression and DataTable expression. Currently data table expressions are limited to a java type. Currently only the implicit type derived from the method argument is used.

For a working example check the calculator examples.
aslakhellesoy pushed a commit to cucumber/cucumber-android that referenced this pull request Jul 4, 2018
 * Implements Cucumber expressions
 * Removes XStream
 * Adds DataTables from cucumber/common#291
 * Introduces the concept of StepExpressions which combines a CucumberExpression and DataTable expression. Currently data table expressions are limited to a java type. Currently only the implicit type derived from the method argument is used.

For a working example check the calculator examples.
@lock
Copy link

lock bot commented Apr 8, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants