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

Read/write table data to a file #3197

Merged
merged 21 commits into from Oct 28, 2022
Merged

Read/write table data to a file #3197

merged 21 commits into from Oct 28, 2022

Conversation

jmfayard
Copy link
Contributor

@jmfayard jmfayard commented Aug 30, 2022

Resolves #3179

See the discussion in the README

I'm starting with readme-oriented programming, documenting how the end result should look like to make sure it makes sense.

Update 1st september needs refactoring but most features are there I think.

Documentation needs updates:

  • it's now file.writeTable(headers, rows) and only works for RowN<String, String, ..., String>
  • use table.mapRows { } if you are using RowN with types other than String
  • map.toTable() is a quick way to produce a table from a map
  • table(headers, list.map { row(...) }) is a quick way to produce a table from a list

Readme-oriented programming to make sure the proposed API make sense
@jmfayard
Copy link
Contributor Author

@Kantis probably need some refactoring and handle variants with 20 generic types, but I think I cover the main use cases now. ouch was lots of work :)

Copy link
Member

@Kantis Kantis left a comment

Choose a reason for hiding this comment

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

Nice work! 🤘 Added various suggestions. Would like a third pair of eyes on this once it's ready 👀

Comment on lines +30 to +33
if (exists().not()) throw AssertionError("Can't read table file")
if (extension != "table") throw AssertionError("Table file must have a .table extension")
val lines = readLines()
if (lines.isEmpty()) throw AssertionError("Table file must have a header")
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to throw IllegalArgumentExceptions instead? No assertion has been made at this point, I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can decide. Personally I don't like exceptions since they don't work well with assertSoftly for example.

   test("invalid file") {
      val file = resourcesDir.resolve("invalid.table") 
      val table = table(headers, file, transform) // implicit assertion that the file exists
      table shouldBe expectedTable
   }

Copy link
Member

Choose a reason for hiding this comment

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

I think there's a difference between an assertion (E.g. validating the output of production code) and a bug in test code. IMO this AssertionError would indicate a fault in the test code, not the main code and I think the distinction is clearer when output as an exception. What do you think?

@jmfayard
Copy link
Contributor Author

jmfayard commented Sep 5, 2022

@Kantis Would like a third pair of eyes on this once it's ready 👀

It's ready-ish now.
I just added the versions with 1 to 9 generic arguments. Fun :)
Who can provide a third pair of eyes?

Also

@sksamuel
Copy link
Member

@Kantis I'm happy if you are.

@Kantis Kantis enabled auto-merge (squash) October 26, 2022 20:37
@Kantis Kantis disabled auto-merge October 26, 2022 20:37
@Kantis
Copy link
Member

Kantis commented Oct 26, 2022

There's a chance we'll try to get #3264 out tonight, so disabled auto-merge for now.

@Kantis Kantis merged commit 632ac87 into kotest:master Oct 28, 2022
@Kantis
Copy link
Member

Kantis commented Oct 28, 2022

Thanks @jmfayard 💯

@Kantis Kantis added this to the 5.6 milestone Oct 28, 2022
@jmfayard
Copy link
Contributor Author

amazing, thanks!

@polarene
Copy link

This is a very cool feature, why isn't it documented? I stumbled upon it randomly while digging through the source code.

@Kantis
Copy link
Member

Kantis commented Jan 30, 2024

@polarene It was documented but only in the 5.4 version of the docs. Fixed in 7b29815

@polarene
Copy link

Thank you! I found out about it because I was trying to come up with something similar, either using Markdown tables or with a DSL (I did a nice hack but it doesn't work if the first value is a number, due to standard lib operators).

But is the page released? Still can't find it in the docs... maybe the index needs to be updated as well?

@polarene
Copy link

Mhh yes, it seems to be missing from these sidebar files:

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.

Read/write Table data to a file format maybe CSV
4 participants