Closed darentsai closed 5 months ago
Hi @darentsai , lets begin with "it's common": I have never seen someone doing something like this and I've seen people doing many strange things. Therefore I have do disagree that it's common ;)
Otherwise it looks good. Just a suggestion, maybe the sorting should also be done via writeData? So that we keep the input and output in sync. Are there any drawbacks that I am overlooking?
Final remarks:
Merging #341 (8286fca) into master (2ace59a) will decrease coverage by
0.08%
. The diff coverage is7.69%
.
@@ Coverage Diff @@
## master #341 +/- ##
==========================================
- Coverage 67.76% 67.67% -0.09%
==========================================
Files 34 34
Lines 8931 8944 +13
==========================================
+ Hits 6052 6053 +1
- Misses 2879 2891 +12
Impacted Files | Coverage Δ | |
---|---|---|
R/build_workbook.R | 97.36% <ø> (ø) |
|
R/writeData.R | 82.90% <7.69%> (-5.44%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 3ceeb84...8286fca. Read the comment docs.
@JanMarvin Thanks for your kind reply. My statements may make some misunderstanding. If the data written to an excel file is used for further analysis or modeling, this feature is not practical. But, if the data have been grouped, summarised by some categorical variables and going to be presented in a report, this feature can make the appearance of table much tidier.(I update a new example in my previous comment using another dataset.)
As for the sorting process, I also thought about doing it in writeData
. In my opinion, a method that prints data to an external file should not change the input structure (e.g. order). Hence, I presume users have arranged data as they want before writing it out. mergeCols
only works on how the data is rendered to excel.
Alright, I agree on the sorting part. Give @ycphs or me a ping when to review (still like to see a test, even if it's just a tiny expect_silent(write...)
).
Hi @darentsai , what is the state of this? Ready and just missing a test or did you find some larger issue that is holding this pull request back?
@JanMarvin Thanks for the reminder. I have tried several kinds of data and adjusted different sets of arguments. I don't find anything wrong so far. I think it's ready to be used but lacks for a test. I'm sorry that I'm not familiar with the test mechanism of package development, so I haven't figured out a proper test example to examine this new feature.
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.
When data have one or more categorical variables, it's common to sort them and merge repeated cells together when creating an excel file. The argument
mergeCols
added inwriteData()
controls which columns need to be merged. Its order indicates how your data is grouped and sorted. An example usingesoph
data is demonstrated as follows: