Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Support for transposed tables #635

Merged
merged 5 commits into from Dec 11, 2013

Conversation

Projects
None yet
3 participants

In reference to issue #382 Provide support for Data Tables with headings along the side this changes add a transpose() and a @Transpose annotation.

Roberto Lo Giacco added some commits Nov 22, 2013

@aslakhellesoy aslakhellesoy commented on an outdated diff Nov 23, 2013

core/src/main/java/cucumber/api/DataTable.java
@@ -63,6 +63,12 @@ public DataTable(List<DataTableRow> gherkinRows, TableConverter tableConverter)
}
this.raw = Collections.unmodifiableList(raw);
}
+
+ private DataTable(List<DataTableRow> gherkinRows, List<List<String>> raw, TableConverter tableConverter) {
@aslakhellesoy

aslakhellesoy Nov 23, 2013

Owner

4 space indent, no tabs please

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Nov 23, 2013

core/src/main/java/cucumber/api/DataTable.java
+ List<T> result = tableConverter.toList(type, this, false);
+ return result;
+ }
+
+ /**
+ * Converts the table to a List of objects. The first column is used to identifies the fields/properties
+ * of the objects.
+ * <p/>
+ * Backends that support generic types can declare a parameter as a List of a type, and Cucumber will
+ * do the conversion automatically.
+ *
+ * @param type the type of the result (should be a {@link List} generic type)
+ * @param <T> the type of each object
+ * @return a list of objects
+ */
+ public <T> List<T> asTransposedList(Type type) {
@aslakhellesoy

aslakhellesoy Nov 23, 2013

Owner

Is this a convenience method for table.transpose().asList(type)?

If that's the case, I don't want this method. We'd have to provide similar convenience methods for the entire interface (asMaps for example) to remain consistent.

@rlogiacco

rlogiacco Nov 23, 2013

This is a leftover, I totally agree with you.

@rlogiacco

rlogiacco Nov 24, 2013

Il giorno sabato 23 novembre 2013, Aslak Hellesøy ha scritto:

In core/src/main/java/cucumber/api/DataTable.java:

  •    List<T> result = tableConverter.toList(type, this, false);
    
  •    return result;
    
  • }
  • /**
  • \* Converts the table to a List of objects. The first column is used to identifies the fields/properties
    
  • \* of the objects.
    
  • \* <p/>
    
  • \* Backends that support generic types can declare a parameter as a List of a type, and Cucumber will
    
  • \* do the conversion automatically.
    
  • *
    
  • \* @param type the type of the result (should be a {@link List} generic type)
    
  • \* @param <T>  the type of each object
    
  • \* @return a list of objects
    
  • */
    
  • public List asTransposedList(Type type) {

Is this a convenience method for table.transpose().asList(type)?

If that's the case, I don't want this method. We'd have to provide similar
convenience methods for the entire interface (asMaps for example) to
remain consistent.

This is a leftover, my bad. I don't even think it's ever used.


Reply to this email directly or view it on GitHubhttps://github.com/cucumber/cucumber-jvm/pull/635/files#r7874900
.

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Nov 23, 2013

core/src/main/java/cucumber/api/DataTable.java
@@ -187,6 +213,29 @@ public TableConverter getTableConverter() {
}
return result;
}
+
+ public DataTable transpose() {
@aslakhellesoy

aslakhellesoy Nov 23, 2013

Owner

See coding conventions in CONTRIBUTING.md

@rlogiacco

rlogiacco Nov 23, 2013

Is it in relation to the 4 spaces? Sorry, CONTRIBUTING.md contains quite a lot of information...

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Nov 23, 2013

core/src/main/java/cucumber/api/DataTable.java
@@ -187,6 +213,29 @@ public TableConverter getTableConverter() {
}
return result;
}
+
+ public DataTable transpose() {
+ List<List<String>> transposed = new ArrayList<List<String>>();
+ for (int i = 0; i < gherkinRows.size(); i++) {
+ Row gherkinRow = gherkinRows.get(i);
+ for (int j = 0; j < gherkinRow.getCells().size(); j++) {
+ List<String> row = null;
+ if (j < transposed.size()) {
+ row = transposed.get(j);
+ }
+ if (row == null) {
+ row = new ArrayList<String>();
+ transposed.add(row);
+ }
+ // fixes non rectangular tables
@aslakhellesoy

aslakhellesoy Nov 23, 2013

Owner

Non-rectangular tables should throw an error. Please add a test that verifies that an error is thrown.

@rlogiacco

rlogiacco Nov 23, 2013

Currently the non transposed version of the table is lenient to cases like the following:

# scenario 1
| a1 | a2 | a3 |
| b1 | b2 |

# scenario 2
| a1 |
| a2 | b2 | c3 |

Do we want to have a different behavior with transposed tables? I don't believe we should have two different approaches here... The non trasposed version doesn't throw an error, it just fills the gaps with empty strings.

@aslakhellesoy

aslakhellesoy Nov 23, 2013

Owner

I think unbalanced tables should always throw an exception, so this would be an opportunity to make it stricter.

@rlogiacco

rlogiacco Nov 24, 2013

Il giorno sabato 23 novembre 2013, Aslak Hellesøy ha scritto:

In core/src/main/java/cucumber/api/DataTable.java:

@@ -187,6 +213,29 @@ public TableConverter getTableConverter() {
}
return result;
}
+

  • public DataTable transpose() {
  •   List<List<String>> transposed = new ArrayList<List<String>>();
    
  •    for (int i = 0; i < gherkinRows.size(); i++) {
    
  •       Row gherkinRow = gherkinRows.get(i);
    
  •       for (int j = 0; j < gherkinRow.getCells().size(); j++) {
    
  •           List<String> row = null;
    
  •           if (j < transposed.size()) {
    
  •               row = transposed.get(j);
    
  •           }
    
  •           if (row == null) {
    
  •               row = new ArrayList<String>();
    
  •               transposed.add(row);
    
  •           }
    
  •           // fixes non rectangular tables
    

Non-rectangular tables should throw an error. Please add a test that
verifies that an error is thrown.

Do we want an exception get generated for all of the following?

scenario 1

| a | b | c | d |
| a | b |

#scenario 2
| a | b |
| a | b | c | d |

Currently these 2 scenarios do not generate an error if the table is not
transposed, that's the reason for my lenient implementation.

Reply to this email directly or view it on GitHubhttps://github.com/cucumber/cucumber-jvm/pull/635/files#r7874915
.

@aslakhellesoy aslakhellesoy commented on the diff Nov 23, 2013

core/src/main/java/cucumber/api/Transpose.java
@@ -0,0 +1,39 @@
+package cucumber.api;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * <p>
+ * This annotation can be specified on step definition method parameters to give Cucumber a hint
@aslakhellesoy

aslakhellesoy Nov 23, 2013

Owner

If DataTable has a transpose() method I'm not convinced we should add an annotation so that the table gets auto-transposed. This complicates the codebase quite a bit for very little benefit. The user can just call transpose() in the stepdef rather than adding an annotation.

I think we should remove the annotation.

@rlogiacco

rlogiacco Nov 23, 2013

Without the @Transpose annotation (I believe Transposed is a better name) how would you achieve something like the following?

  Given the following person
    | first name    | Roberto    |
    | last name     | Lo Giacco  |
    | date of birth | 1975-11-25 |

The above table can be matched by this step def method

@Given("^the following person$")
public void the_following_person(@Transpose Person person) {
  //...
}

Without it you need to...

@Given("^the following person$")
public void the_following_person(DataTable person) {
  Person p = person.transpose().convert(Person.class);
  // or, with the method I needed to add to support the annotation...
  Person p = person.convert(Person.class, true);
  //...
}

If you are convinced the annotation is not needed I can back out that commit

@rlogiacco

rlogiacco Nov 24, 2013

With regard to this, without the annotation there's no way you can declare
a bean or list to get its definition transposed.

With the annotation

public void myMethod(@transpose User user) {
...
}

Without the annotation
public void myMethod(DataTable user) {
user = user.transpose();
// how can we get the user type now?
}

Il giorno sabato 23 novembre 2013, Aslak Hellesøy ha scritto:

In core/src/main/java/cucumber/api/Transpose.java:

@@ -0,0 +1,39 @@
+package cucumber.api;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**

  • *

  • * This annotation can be specified on step definition method parameters to give Cucumber a hint

If DataTable has a transpose() method I'm not convinced we should add an
annotation so that the table gets auto-transposed. This complicates the
codebase quite a bit for very little benefit. The user can just call
transpose() in the stepdef rather than adding an annotation.

I think we should remove the annotation.


Reply to this email directly or view it on GitHubhttps://github.com/cucumber/cucumber-jvm/pull/635/files#r7874925
.

Pushed two more commits trying to fulfill code conventions and to implement table balancing verification as suggested. Transpose annotation is still present as I believe it provides some nice feature.

@aslakhellesoy Is anything I can do here to move this forward?

Owner

aslakhellesoy commented Dec 11, 2013

LGTM. @brasmusson can you take a second look and merge this in?

Contributor

brasmusson commented Dec 11, 2013

@aslakhellesoy Yes, I'll will take care of it.

@brasmusson brasmusson merged commit 0c37374 into cucumber:master Dec 11, 2013

1 check passed

default The Travis CI build passed
Details
Contributor

brasmusson commented Dec 11, 2013

Merged. Thanks for your contribution @rlogiacco.

Proud of having had the opportunity to help!

;)

Il giorno mercoledì 11 dicembre 2013, Björn Rasmusson ha scritto:

Merged. Thanks for your contribution @rlogiaccohttps://github.com/rlogiacco
.


Reply to this email directly or view it on GitHubhttps://github.com/cucumber/cucumber-jvm/pull/635#issuecomment-30362654
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment