Add validation for selector names to prevent browser rendering failures. #246
Conversation
ANAMASGARD
force-pushed
the
fix/invalid-selector-character
branch
2 times, most recently
from
013965b to
32e15c5
Compare
- Update NEWS.md to reference PR#246 instead of Issue animint#232 - Refactor checkSelectorNames() to use sprintf instead of paste0 - Remove unnecessary if(length==0) check and explicit return(NULL) - Remove all empty lines from test_that blocks per style guidelines
ANAMASGARD
force-pushed
the
fix/invalid-selector-character
branch
from
44a793b to
258284c
Compare
…edback Changed error message construction in checkSelectorNames() to use one long literal string with \n newlines instead of sprintf with multiple %s placeholders, following best practices from CRAN's potools vignette.
|
✅ Uses one long literal string with \n newlines |
|
hi please read my comments and push some fixes. also can you please avoid using AI code generation? (Github copilot etc) |
|
Ok Sir, I just wanted to ask as I am the developer member of this org so should I directly push commits on the master branch on this repo. |
|
please avoid pushing to master branch. |
|
no please do not merge your branch with master unless I approve. please read https://github.com/animint/animint2/blob/master/CONTRIBUTING.md#prs-from-forks-versus-branches-in-this-repo |
|
Sir I wanted to ask can you please review this PR and tell what changes I should be making so that test pass . |
|
I'm waiting for you to address this review comment #246 (comment) Did you see that? In general to emphasize the review comments that are still relevant, you should click on "Resolve conversation" for the ones that are already addressed. |
Address review comment to simplify nested sapply calls and fix test typo.
ANAMASGARD
force-pushed
the
fix/invalid-selector-character
branch
from
b7f425b to
4277d24
Compare
|
Sir @tdhock,
Thank you . |
|
please click "Resolve conversation" on old comments, after pushing fixes. |
R/z_animintHelpers.R
Outdated
| ## Most problematic: # (hash/id selector), . (class selector), : (pseudo-class) | ||
| ## We check for these common problematic characters | ||
| invalid.chars <- c("#", "!", "@", "$", "%", "^", "&", "*", "=", "|", "\\", "/", "'", "\"", "`", "?", "<", ">", "(", ")", "[", "]") | ||
|
|
There was a problem hiding this comment.
please undo addition of empty lines
R/z_animintHelpers.R
Outdated
| has.invalid <- vapply(selector.names, function(name) { | ||
| any(vapply(invalid.chars, grepl, logical(1), x = name, fixed = TRUE)) | ||
| }, logical(1)) | ||
|
|
||
| if(any(has.invalid)){ | ||
| invalid.names <- selector.names[has.invalid] | ||
| ## Find which character(s) are problematic for each name | ||
| problematic <- sapply(invalid.names, function(name) { | ||
| found.chars <- invalid.chars[sapply(invalid.chars, grepl, name, fixed = TRUE)] | ||
| paste0("'", found.chars, "'", collapse = ", ") |
There was a problem hiding this comment.
please remove sapply, which is not necessary
just use grep or grepl
There was a problem hiding this comment.
and remove logical(1) which is confusing. Where did you learn that? why not just use FALSE instead?
> logical(1)
[1] FALSE
ANAMASGARD
force-pushed
the
fix/invalid-selector-character
branch
from
05b8283 to
32adade
Compare
2 participants
Fixes #232
I First reproduced the test locally which you gave in hte issue :-
nearest neighbors :--- blank browser page, no error. "Before"
Then I added R-side validation function
checkSelectorNames()that:Changes made in these files :-
R/z_animintHelpers.R: NewcheckSelectorNames()validation functionR/z_animint.R: Call validation after selectors are populatedtests/testthat/test-compiler-invalid-selector-characters.R: 9 comprehensive testsNEWS.md: Documentation of new feature.After fix:
"# nearest neighbors"→ clear R error, suggests solutionI also ran Rscript in terminal for verification (below is the screenshot ):-
Commit Message Reference
The commit message mentions
(#232)which links this PR to the issue for automatic tracking.