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

test diamonds: reduce needless converters #205

Merged

Conversation

tikkss
Copy link
Contributor

@tikkss tikkss commented Aug 31, 2024

GitHub: GH-188

Because date_time is not included in the following csvs.

  • diamonds.csv (diamonds dataset inherits from Ggplot2Dataset)
  • mpg.csv (fuel economy dataset inherits from Ggplot2Dataset)

Before this change:

$ time ruby test/run-test.rb -t DiamondsTest --verbose=important-only
Finished in 3.702616 seconds.
2 tests, 2 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0
notifications

real    0m4.665s
user    0m4.136s
sys     0m0.226s

After this change:

$ time ruby test/run-test.rb -t DiamondsTest --verbose=important-only
Finished in 3.367821 seconds.
2 tests, 2 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0
notifications

real    0m4.179s
user    0m3.738s
sys     0m0.202s

After this change, the fuel economy dataset test also passed:

$ ruby test/run-test.rb -t FuelEconomyTest --verbose=important-only
Finished in 0.017332 seconds.
2 tests, 2 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications

GitHub: red-data-toolsGH-188

Because date_time is not included in the following csvs.

* diamonds.csv (diamonds dataset inherits from `Ggplot2Dataset`)
* mpg.csv (fuel economy dataset inherits from `Ggplot2Dataset`)

Before this change:

```console
$ time ruby test/run-test.rb -t DiamondsTest --verbose=important-only
Finished in 3.702616 seconds.
2 tests, 2 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0
notifications

real    0m4.665s
user    0m4.136s
sys     0m0.226s
```

After this change:

```console
$ time ruby test/run-test.rb -t DiamondsTest --verbose=important-only
Finished in 3.367821 seconds.
2 tests, 2 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0
notifications

real    0m4.179s
user    0m3.738s
sys     0m0.202s
```

After this change, the fuel economy dataset test also passed:

```console
$ ruby test/run-test.rb -t FuelEconomyTest --verbose=important-only
Finished in 0.017332 seconds.
2 tests, 2 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
```
@kou
Copy link
Member

kou commented Sep 1, 2024

There are more datasets in ggplot2: https://ggplot2.tidyverse.org/reference/#data

Could you check whether all datasets don't have a date_time field or not?

@tikkss
Copy link
Contributor Author

tikkss commented Sep 1, 2024

Sure! I'll check the following unavailable datasets. Thanks!

  • Available datasets
    • diamonds
    • mpg
  • Unavailable datasets
    • economics economics_long
    • faithfuld
    • midwest
    • msleep
    • presidential
    • seals
    • txhousing
    • luv_colours

@kou
Copy link
Member

kou commented Sep 1, 2024

Ah, Ggploat2Dataset requires defining COLUMN_NAME_MAPPING in subclasses. So other datasets are unavailable now.

OK. Let's use this change. Let's add more converters again when we add support for other datasets and they need more converters.

@kou kou merged commit dd2e617 into red-data-tools:master Sep 1, 2024
12 checks passed
@tikkss tikkss deleted the test-diamonds-reduce-needless-converters branch September 1, 2024 21:32
@tikkss
Copy link
Contributor Author

tikkss commented Sep 1, 2024

Thanks for your review!

OK. Let's use this change. Let's add more converters again when we add support for other datasets and they need more converters.

OK. I see.

@tikkss
Copy link
Contributor Author

tikkss commented Sep 2, 2024

By the way, Could I add the unavailable datasets to the Issues?

@kou
Copy link
Member

kou commented Sep 2, 2024

Yes, please.

@tikkss
Copy link
Contributor Author

tikkss commented Sep 2, 2024

Sure!

@tikkss
Copy link
Contributor Author

tikkss commented Sep 17, 2024

Add the following unavailable datasets to the issues.

#209, #214, #218, #223, #226, #229, #230, #231

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