More helpful error message for invalid formats #67

zdz2101 · 2022-12-21T18:52:02Z

Per #60 and #64 , users seem to encounter this error with relative frequency:

"Error: Writing failure: A provided format string could not be understood."

I believe this is actually a haven error-message coming from line 55 in R/write.R :
write_xpt(data, path = path, version = 5, name = name)

To remedy this, I propose to add an additional format check inside xpt_validate() which is called at line 47 of R/write.R, but changes are contained inside R/utils-xportr.R

codecov · 2022-12-23T19:55:48Z

Codecov Report

Merging #67 (9f4c146) into main (e598735) will increase coverage by 2.66%.
The diff coverage is 83.78%.

❗ Current head 9f4c146 differs from pull request most recent head ba4989f. Consider uploading reports for the commit ba4989f to get more accurate results

@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
+ Coverage   66.18%   68.85%   +2.66%     
==========================================
  Files          10       10              
  Lines         349      366      +17     
==========================================
+ Hits          231      252      +21     
+ Misses        118      114       -4

Impacted Files	Coverage Δ
R/utils-xportr.R	`68.81% <83.78%> (+10.92%)`	⬆️
R/write.R	`83.33% <0.00%> (+5.55%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

zdz2101 · 2022-12-23T20:03:34Z

@elimillera @bms63 Not quite sure how to diagnose the R-CMD-checks, besides that what do you think of these proposed changes?

bms63 · 2023-01-05T21:56:24Z

Hey @zdz2101 I think I fixed the actions for linux os. One is still running as I write

bms63 · 2023-01-05T21:56:57Z

R/utils-xportr.R

+fmt_fmts <- function(x) {
+  fmts <- ntext(length(x), "Format", "Formats")
+  glue("{fmts} {encode_vals(x)}")
+}
+
+


is this used somewhere? I see it is new but don't see it being used?

Used up around line 114

oh lol i see can you move it so it comes before that call

If you scroll up a bit, you'll see a few other functions that are also mentioned after where they are called, do you want me to reorganize the entirety of the file such that functions are created before where they are called?

Please just to increase readability for those who come afterwards.

bms63

Can you roxygen this file and make it so it doesn't export. Sorry you have to clean up our mess!!

elimillera

Thanks @zdz2101! I'll be pushing up a some changes to the news file and description but otherwise I think this looks great.

bms63 · 2023-01-06T19:31:46Z

@zdz2101 Sorry. The newly roxygened functions shouldn't be exported unless you think they might be useful to users. I just wanted titles, explanations of function purpose (if you think it is not clear to a new developer) and arguments described properly. Like we do in admiral

DESCRIPTION

bms63 · 2023-01-06T19:38:32Z

NEWS.md

@@ -1,3 +1,6 @@
+# xportr 0.1.0.9000 (development)
+* Added a new validation test that errors when users pass invalid formats. Thanks to @zdz2101!


Can we link the issue here as well?

zdz2101 · 2023-01-06T19:41:13Z

@zdz2101 Sorry. The newly roxygened functions shouldn't be exported unless you think they might be useful to users. I just wanted titles, explanations of function purpose (if you think it is not clear to a new developer) and arguments described properly. Like we do in admiral

Ah I see I thought you meant it just isn't exported in the NAMESPACE, which it currently isn't. Although, I think by using roxygen-style comments and a command like devtools::document() it expects to create .Rd files and request appropriate entries in the reference section under the pkgdown.yml . I can leave the roxygen comments and use something like "@"noRd ?

bms63 · 2023-01-06T19:43:06Z

@zdz2101 Sorry. The newly roxygened functions shouldn't be exported unless you think they might be useful to users. I just wanted titles, explanations of function purpose (if you think it is not clear to a new developer) and arguments described properly. Like we do in admiral

Ah I see I thought you meant it just isn't exported in the NAMESPACE, which it currently isn't. Although, I think by using roxygen-style comments and a command like devtools::document() it expects to create .Rd files and request appropriate entries in the reference section under the pkgdown.yml . I can leave the roxygen comments and use something like "@"noRd ?

Apologies for creating confusion. Yes I meant noRd that is my bad.

bms63

I think this is looking great. Thanks a lot @zdz2101!! Just the version comment needs to be addressed by @elimillera

NEWS.md

Zelos Zhu added 2 commits December 21, 2022 18:24

#64 - First draft to add format check in xpt_validate

d75b9b6

Add a test for a formatting error

475abab

This was linked to issues Dec 21, 2022

More informative error message for invalid formats #64

Closed

xportr_format issue from Roche testing #60

Closed

#64 - Additional acceptable date formats

aed23ac

zdz2101 requested review from elimillera and bms63 December 23, 2022 20:02

bms63 added 2 commits January 5, 2023 16:34

Update check-standard.yaml

9fe74e1

Update check-standard.yaml

09809dc

bms63 reviewed Jan 5, 2023

View reviewed changes

elimillera approved these changes Jan 6, 2023

View reviewed changes

Update news and dev version

db7e2d1

zdz2101 requested a review from bms63 January 6, 2023 19:16

bms63 reviewed Jan 6, 2023

View reviewed changes

DESCRIPTION Outdated Show resolved Hide resolved

bms63 reviewed Jan 6, 2023

View reviewed changes

zdz2101 force-pushed the feature/error-message-for-invalid-formats branch from ba70a36 to db7e2d1 Compare January 6, 2023 20:06

Use noRd tag for utils-xportr.R

0c53d5e

zdz2101 requested a review from bms63 January 6, 2023 20:13

Add issue number to news

374eca6

bms63 approved these changes Jan 8, 2023

View reviewed changes

Zelos Zhu and others added 2 commits January 9, 2023 15:17

Add Zelos as Developer

ae907d9

Update DESCRIPTION to version 0.2.0

0359f18

zdz2101 commented Jan 10, 2023

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

zdz2101 marked this pull request as ready for review January 10, 2023 16:49

Update NEWS.md to match DESCRIPTION

ba4989f

zdz2101 merged commit d1b33be into main Jan 10, 2023

mstackhouse mentioned this pull request Feb 3, 2023

devel and main branch seem out of sync #68

Closed

bms63 deleted the feature/error-message-for-invalid-formats branch April 23, 2023 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More helpful error message for invalid formats #67

More helpful error message for invalid formats #67

zdz2101 commented Dec 21, 2022 •

edited

Loading

codecov bot commented Dec 23, 2022 •

edited

Loading

zdz2101 commented Dec 23, 2022

bms63 commented Jan 5, 2023

bms63 Jan 5, 2023

zdz2101 Jan 5, 2023

bms63 Jan 5, 2023

zdz2101 Jan 6, 2023

bms63 Jan 6, 2023

bms63 left a comment

elimillera left a comment

bms63 commented Jan 6, 2023 •

edited

Loading

bms63 Jan 6, 2023

zdz2101 commented Jan 6, 2023 •

edited

Loading

bms63 commented Jan 6, 2023

bms63 left a comment

		@@ -1,3 +1,6 @@
		# xportr 0.1.0.9000 (development)
		* Added a new validation test that errors when users pass invalid formats. Thanks to @zdz2101!

More helpful error message for invalid formats #67

More helpful error message for invalid formats #67

Conversation

zdz2101 commented Dec 21, 2022 • edited Loading

codecov bot commented Dec 23, 2022 • edited Loading

Codecov Report

zdz2101 commented Dec 23, 2022

bms63 commented Jan 5, 2023

bms63 Jan 5, 2023

Choose a reason for hiding this comment

zdz2101 Jan 5, 2023

Choose a reason for hiding this comment

bms63 Jan 5, 2023

Choose a reason for hiding this comment

zdz2101 Jan 6, 2023

Choose a reason for hiding this comment

bms63 Jan 6, 2023

Choose a reason for hiding this comment

bms63 left a comment

Choose a reason for hiding this comment

elimillera left a comment

Choose a reason for hiding this comment

bms63 commented Jan 6, 2023 • edited Loading

bms63 Jan 6, 2023

Choose a reason for hiding this comment

zdz2101 commented Jan 6, 2023 • edited Loading

bms63 commented Jan 6, 2023

bms63 left a comment

Choose a reason for hiding this comment

zdz2101 commented Dec 21, 2022 •

edited

Loading

codecov bot commented Dec 23, 2022 •

edited

Loading

bms63 commented Jan 6, 2023 •

edited

Loading

zdz2101 commented Jan 6, 2023 •

edited

Loading