TrackA_RMedicine_Hackathon_Apr2026#24#31
Conversation
peymaneshghi
changed the title
|
Resolves #24 |
kpagacz
left a comment
kpagacz
left a comment
There was a problem hiding this comment.
Thank you very much for your contribution!
I attached some general comments about the submitted PR and I will let Przemek handle the specifics.
| quoted_vals <- paste0("'", cond$value, "'", collapse = ", ") | ||
| paste0("c(", quoted_vals, ")") |
There was a problem hiding this comment.
Please use sprintf instead of paste0 and paste in cases where the input is not vectorized.
There was a problem hiding this comment.
I don't think this comment or the rest of the comments to use sprintf were addressed in any way. Are you against using sprintf here?
There was a problem hiding this comment.
No , I agree with using sprintf and I have done the updates.
| input[[paste0("operator_selector_", current_id)]], | ||
| { | ||
| op <- input[[paste0("operator_selector_", current_id)]] | ||
|
|
||
| output[[paste0("value_input_", current_id)]] <- renderUI({ | ||
| if (op %in% c("%in%", "!%in%")) { | ||
| shinyWidgets::pickerInput( | ||
| session$ns(paste0("value_input_", current_id)), |
There was a problem hiding this comment.
This should require the input operator otherwise this renders nothing, right?
There was a problem hiding this comment.
Yes, I have added req(op) but it was still rendering fine. However, I have noticed a bug in this chunk. Will update it by tomorrow
There was a problem hiding this comment.
req(op) is now removed as it's not needed with the current change to column selector observer.
|
The documentation of |
|
Hi @kpagacz, thank you for the review. I agree there is unnecessary styling added. It is due to the AIR formatter enabled in my Positron that added these changes. |
Siddhesh2097
force-pushed
the
24_add_more_condn
branch
from
2606323 to
7cb4122
Compare
kpagacz
left a comment
kpagacz
left a comment
There was a problem hiding this comment.
Have a look at the comments, I hope the resolutions were automatic and not done manually.
| setMethod("add_condition", "BlockConditions", function(object, variable, operator, value) { | ||
| object@conditions <- c(object@conditions, list(list(variable = variable, operator = operator, value = value))) | ||
| object | ||
| object |
| quoted_vals <- paste0("'", cond$value, "'", collapse = ", ") | ||
| paste0("c(", quoted_vals, ")") |
There was a problem hiding this comment.
I don't think this comment or the rest of the comments to use sprintf were addressed in any way. Are you against using sprintf here?
| cond_str <- if (operator == "%in%") { | ||
| quoted_vals <- paste0("'", value, "'", collapse = ", ") | ||
| paste0(variable, " %in% c(", quoted_vals, ")") | ||
| } else if (operator == "!%in%") { | ||
| quoted_vals <- paste0("'", value, "'", collapse = ", ") | ||
| paste0("!", variable, " %in% c(", quoted_vals, ")") | ||
| } else { | ||
| paste0(variable, " ", operator, " ", value) | ||
| } |
There was a problem hiding this comment.
This was not addressed in any way. Are you against using a switch statement here?
| }) | ||
|
|
||
| # Update choices for values for the new block | ||
| #Update choices for values for the new block |
There was a problem hiding this comment.
This comment was not addressed and on this I will insist.
| } else if (is.character(col_data) || is.factor(col_data)) { | ||
| lvls <- levels(factor(col_data)) | ||
|
|
||
| #Observe operator changes to change to single / multi select |
There was a problem hiding this comment.
This comment was not addressed and I will insist on this
Updated |
|
@kpagacz To summarise, updates were done from my end but were not pushed as I discovered a bug. In my latest commit, all your comments have been addressed. And as I mentioned, there is a bug due to my addition of %in% operator where value levels are not updated, I'll try to fix it by tomorrow. Few other add comments and anything missed will be addressed in my next commit. I generally request for a re-review once I'm done with my updates, and unfortunately you had to see my unfinished work. |
kpagacz
left a comment
kpagacz
left a comment
There was a problem hiding this comment.
I tested this and it works as I would expect it, thank you so much for your contribution of this feature and also being patient with general code improvements to this repsitory in reviews :)
You are joining the hall of fame of contributors in our NEWS file!
3 participants
Added %in% and !%in% conditions to character and filter block.