Add input validation for setter and read functions #231

maxloeffler · 2023-01-10T13:00:42Z

Prerequisites

I adhere to the coding conventions (described here) in my code.
I have updated the copyright headers of the files I have modified.
I have written appropriate commit messages, i.e., I have recorded the goal, the need, the needed changes, and the location of my code modifications for each commit. This includes also, e.g., referencing to relevant issues.
I have put signed-off tags in all commits.
I have updated the changelog file NEWS.md appropriately.
I have checked whether I need to adjust the showcase file showcase.R with respect to my changes.
The pull request is opened against the branch dev.

Description

Add input validation into setter functions and functions that read input from files.
Fix check for empty input files in almost all read functions

Changelog

Add the 'verify.data.frame.column' to the miscellaneous utilities to check the existence and welltypedness of columns in a dataframe. This works towards fixing se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

Add call to 'verify.data.frame.columns' to relevant setters and read functions This works towards fixing se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

Change most internal types from numeric to integer; also in related tests. Add sufficient checks for empty input-files in read functions. Close previously unlocked parenthesis in util-data. Change dummy fill of author email from NA to null-string to comply with character type of column. This fixes se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

Add copyright header to updated utility and test files. This fixes se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

bockthom

Thanks @MaLoefUDS, this PR looks really good so far! I only spotted a few inconsistencies in comments that should be fixed, please see my individual comments below.

In addition, there is one data source we have not talked about yet: The custom event timestamps. We also should validate the inputs for its setter and read functions, although it is a little bit different than the other data sources:

read.custom.event.timestamps: This function reads a data.frame with two columns – one is of type character, the other one of type POSIXct. We should check this directly after reading it. However, there are no constants and no column names, as this data format is not used outside of the read function. So there should be only hardcoded checks within this function. Because at the end, the read data.frame is transferred to a named list prior to returning.

set.custom.event.timestamps: This function just takes a list as parameter. So we should just check whether the input is a list.

So, could you please add the validation for these two special cases?

~~And please don't forget to add the description for this PR (you don't need to add a changelog there).~~ (description is already present, I just have overlooked it, sorry)

util-misc.R

tests/test-misc.R

util-read.R

tests/test-misc.R

Add check that input is a list in 'set.custom.event.timestamps' and verify correct input structure in 'read.custom.event.timestamps'. Concretize unclear comments, fix punctuation and spelling mistakes. Update copyright headers. This fixes feedback on se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

bockthom

I just spotted two issues regarding the new checks for the custom event timestamps. Please fix them, everything else looks good to me.

(And don't worry about the failing CI pipeline on GitHub. There are some temporary problems at our CI provider that let the CI pipeline always run into a timeout before starting the tests. Just ignore the pipeline for the moment and just make sure that the tests run successfully on your end.)

util-read.R

hechtlC

Looks good, thanks @MaLoefUDS . I just found one line that has more than 120 characters. Once this is fixed and the annotations by @bockthom are processed, we can merge this.

util-misc.R

Remove check on first column of timestamp data in 'read.custom.event.timestamps' as non-character fields will be cast to character anyways by call to 'names'. Simplify POSIXct format check on second column using 'get.date.from.string' function. Wrap overly long line in 'verify.data.frame.columns'. This fixes feedback on se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

Add changes and additions reguarding dataframe verification to 'NEWS.md'. This completes checklist on se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

bockthom

Looks good, thanks!

(I only spotted one minor indentation inconsistency)

util-misc.R

Indent line-wrapped string according to convention. This fixes feedback on se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

NEWS.md

In change issue number to pull-request number and add comma after. This fixes feedback on se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

hechtlC

I have just tried to run the test suite on my machine since the CI is still stuck. And I noticed that you are using an experimental feature of testthat in tests-misc.R. See my comment.
Once this is fixed for all four occurences, I will merge the PR. Thanks again for the great work!

hechtlC · 2023-01-18T13:10:40Z

tests/test-misc.R

+    data.frame = data.frame(user.names, age, is.male)
+
+    ## 1) Check base functionality (benign use-case)
+    expect_no_error(verify.data.frame.columns(


Please change this to expect_error(..., NA, message(...)).
The expect_no_error function is a new feature from testthat but is still experimental. So for now we want to stay at the "old" version. Also please put a comment behind the check with Expect no error.

I didn't know about this, I will do the replacement 😇

Replace 'expect_no_error(expr, message)' by 'expect_error(expr, NA, message)'. This fixes feedback on se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

maxloeffler added 4 commits December 24, 2022 17:21

Add function 'verify.data.frame.columns'

d1d9a03

Add the 'verify.data.frame.column' to the miscellaneous utilities to check the existence and welltypedness of columns in a dataframe. This works towards fixing se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

Add dataframe structure verification

b7a9588

Add call to 'verify.data.frame.columns' to relevant setters and read functions This works towards fixing se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

Add copyright header

8043e90

Add copyright header to updated utility and test files. This fixes se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

bockthom reviewed Jan 12, 2023

View reviewed changes

bockthom added this to the v4.3 milestone Jan 12, 2023

bockthom requested changes Jan 16, 2023

View reviewed changes

util-read.R Outdated Show resolved Hide resolved

util-read.R Outdated Show resolved Hide resolved

hechtlC previously approved these changes Jan 17, 2023

View reviewed changes

util-misc.R Outdated Show resolved Hide resolved

maxloeffler added 2 commits January 18, 2023 13:02

Update 'NEWS.md'

4ff6b53

Add changes and additions reguarding dataframe verification to 'NEWS.md'. This completes checklist on se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

maxloeffler dismissed hechtlC’s stale review via 4ff6b53 January 18, 2023 12:23

bockthom reviewed Jan 18, 2023

View reviewed changes

util-misc.R Outdated Show resolved Hide resolved

Fix indentation

1f10f8f

Indent line-wrapped string according to convention. This fixes feedback on se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

bockthom reviewed Jan 18, 2023

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

Change issue number to pull-request number in

fc52984

In change issue number to pull-request number and add comma after. This fixes feedback on se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

bockthom previously approved these changes Jan 18, 2023

View reviewed changes

hechtlC requested changes Jan 18, 2023

View reviewed changes

Replace experimental 'testthat' feature

38c2077

Replace 'expect_no_error(expr, message)' by 'expect_error(expr, NA, message)'. This fixes feedback on se-sic#208. Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>

maxloeffler dismissed bockthom’s stale review via 38c2077 January 18, 2023 14:03

hechtlC approved these changes Jan 18, 2023

View reviewed changes

hechtlC merged commit 3c452ca into se-sic:dev Jan 18, 2023

maxloeffler deleted the dev branch January 18, 2023 14:34

bockthom mentioned this pull request Apr 17, 2023

Version 4.3 #243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add input validation for setter and read functions #231

Add input validation for setter and read functions #231

maxloeffler commented Jan 10, 2023 •

edited

Loading

bockthom left a comment •

edited

Loading

bockthom left a comment

hechtlC left a comment

bockthom left a comment

hechtlC left a comment

hechtlC Jan 18, 2023

maxloeffler Jan 18, 2023

Add input validation for setter and read functions #231

Add input validation for setter and read functions #231

Conversation

maxloeffler commented Jan 10, 2023 • edited Loading

Prerequisites

Description

Changelog

bockthom left a comment • edited Loading

Choose a reason for hiding this comment

bockthom left a comment

Choose a reason for hiding this comment

hechtlC left a comment

Choose a reason for hiding this comment

bockthom left a comment

Choose a reason for hiding this comment

hechtlC left a comment

Choose a reason for hiding this comment

hechtlC Jan 18, 2023

Choose a reason for hiding this comment

maxloeffler Jan 18, 2023

Choose a reason for hiding this comment

maxloeffler commented Jan 10, 2023 •

edited

Loading

bockthom left a comment •

edited

Loading