Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request for renames in teal_data_module #1323

Closed
pawelru opened this issue Aug 14, 2024 · 2 comments · Fixed by #1430
Closed

Request for renames in teal_data_module #1323

pawelru opened this issue Aug 14, 2024 · 2 comments · Fixed by #1430
Assignees
Labels

Comments

@pawelru
Copy link
Contributor

pawelru commented Aug 14, 2024

Follow-up on #1253

Please find below a list of objects which names I personally found confusing. If you agree - let's try to find a good name.

  • srv_teal_data() & ui_teal_data()
    This is a module to resolve teal_data_module class of objects, in particular: not a teal_data class of objects
  • inside srv_teal_data(): it's easy to confuse data, data_module and data_out (for this: there is srv_validate_reactive_teal_data(..., data = data_out, ...) which is even more confusing)
    It's fully functional but it's very easy to miss what object means what and you have to put a lot of extra focus to correctly follow the logic. I propose the following (open for discussion):
    • rename data to r_teal_data (reactive containing teal_data)
    • rename data_module to teal_data_module
    • rename data_out into module_out (to keep it separate from data but close to teal_data_module)
  • inside srv_validate_reactive_teal_data(): rename data argument into r_teal_data (reactive containing teal_data)
  • inside srv_validate_reactive_teal_data(): rename data_out_r variable into r_try_teal_data (reactive containing try-catch of r_teal_data evaluation (see above))
  • inside .fallback_on_failure():
    • rename this argument into x
    • rename that argument into y
      this and that are just bad arg names. This feels very similar to %>% or rlang::%||% inflix functions (with exception of label arg) and these usually have x and y
@llrs-roche
Copy link
Contributor

This issue is planned for these weeks. Should we @insightsengineering/nest-core-dev have a discussion today or tomorrow to see what/how do we rename and the plan for it?

@llrs-roche
Copy link
Contributor

Some comments/thoughts to avoid forgetting these:

  • srv_teal_data() & ui_teal_data(): I'm not sure I understand the proposed change: I'll check the code in more detail.
  • inside srv_teal_data(): as far as I could test these are the easy changes (on tmc and tmg we are using a suffix instead of a prefix to signal it is reactive so I would use teal_data_r)
  • srv_validate_reactive_teal_data(): If we keep the parameter the same but change only the internals we have potentially more confusion but less breakage. Renaming some parameters would lead to having to change a lot of code.
  • inside .fallback_on_failure(): The code has changed meanwhile and there is no longer .fallback_on_failure()

PR to track updates: #1430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants