Skip to content

#1523 progress: adds option for dplyr-inspired column class summary with printing #1529

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

Merged
merged 1 commit into from
Mar 4, 2016
Merged

#1523 progress: adds option for dplyr-inspired column class summary with printing #1529

merged 1 commit into from
Mar 4, 2016

Conversation

MichaelChirico
Copy link
Member

Tasks 1 and 8 of #1523 (will incrementally fill out the .Rd as the other features are added).

Judgment calls (not married to any of them, so happy to change):

  • List of common classes to support, and their abbreviation:
    • Anything not on this list will simply print the full class in angle brackets. Hopefully rare.
c(list = "<list>", integer = "<int>", numeric = "<num>",
  character = "<char>", Date = "<Date>", complex = "<cplx>",
  factor = "<fctr>", POSIXct = "<POSc>", logical = "<lgcl>",
  IDate = "<IDat>", integer64 = "<i64>", raw = "<raw>",
  expression = "<expr>")
  • Using angle brackets instead of the parentheses used by dplyr. This was basically to match the existing formatting done to list columns.
  • Not printing anything about the row indices to indicate that row is column types (could have printed "Classes:" or something to that effect)
  • also stayed on the minimal side in the .Rd.

@arunsrinivasan
Copy link
Member

Great! I think ordered factor is the only one missing?

Like points 1 and 2. I think it's self-evident on 3, but I don't have a preference either.

@arunsrinivasan
Copy link
Member

Seems excellent. But build fails on that test.. If you're not already aware, use test.data.table() to check if all tests pass.

Another error from Examples. print.data.table won't work since it's not exported?? Why not print()?

@MichaelChirico
Copy link
Member Author

@arunsrinivasan I used test.data.table, it only worked on my machine though. My test data was too wide, so whether/where it wraps is machine-dependent... splitting into two tests and re-pushing soon.

@arunsrinivasan
Copy link
Member

@MichaelChirico when you fix and rebase, ping me, I'll merge. 👍

@MichaelChirico
Copy link
Member Author

@arunsrinivasan test.data.table is running fine for me, not sure what the "conflicts that must be resolved" are... seems Travis build won't go through yet so I'm not 100% that my fix to the tests worked

@arunsrinivasan
Copy link
Member

Did you pull to get to the latest commit? I guess it's because of the last two issues I closed.

attempting test fix again

gave up on second test

fixing error in .Rd
@MichaelChirico
Copy link
Member Author

@arunsrinivasan finally got it to pass muster. Did away with one of the tests that was being stubborn; the remaining test gets the point across I think.

@arunsrinivasan
Copy link
Member

@MichaelChirico looks great. For the future, it'd be nice to have a PR for a single issue..
Could you share as to which test was removed? Perhaps we should try and see if things need to be fixed.

Will merge this, and take a look at the test after.

arunsrinivasan added a commit that referenced this pull request Mar 4, 2016
#1523 progress: adds option for dplyr-inspired column class summary with printing
@arunsrinivasan arunsrinivasan merged commit 58c091a into Rdatatable:master Mar 4, 2016
@MichaelChirico
Copy link
Member Author

@arunsrinivasan it's been a while, sadly can't remember which test it was...

about single PRs, I'm getting conflicting messages from you... See comments in #1523:

Me: should I try and PR this one issue at a time? Or in a fell swoop? I've got 8 basically taken care of, just need to add tests.

You: Michael, separate PRs.

Could you clarify? Happy to follow whichever you prefer.

@MichaelChirico MichaelChirico deleted the print.data.table branch March 4, 2016 21:02
@arunsrinivasan
Copy link
Member

@MichaelChirico, Sure. I mean, separate PRs == 1 PR for 1 issue. The consolidation of all other print related issues is for us to easily find them in one place. They do not represent one big issue. Is that the confusion?

@MichaelChirico
Copy link
Member Author

@arunsrinivasan hmm still not 100% sure I'm with you.

How about this:

For the remaining 8 bullet points of #1523, should I file 8 PRs, or just 1?

@arunsrinivasan
Copy link
Member

8 bullet points = 8 separate issues = 8 separate PRs. They are consolidated there for convenience. #1523 does not represent one giant issue.

@MichaelChirico
Copy link
Member Author

@arunsrinivasan about PRs, basically with you, but the 8 points there represent only 6 issues (since points 5-7 address different parts of #645). Should I submit one PR for 5-7, or three?

@arunsrinivasan
Copy link
Member

#645 is from R-Forge where multiple issues got filed together too often. Those points seem to be 4 different enhancements AFAICT.

It has very less to do with how issues are filed. Separate commits help debug the issue later / fix them if there's a bug/regression and not having to search as to which code part corresponded to which fix.

@arunsrinivasan
Copy link
Member

@MichaelChirico I've already taken care of it.. pushing a commit with minor changes.

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