Remove support for dispatchers; add early error feedback; tweak test to make it a bit more solid #11
Conversation
Update ci.yml
| linters <- lintr::default_linters # -[ diff with dv.templates 3ca8d7a10cfc7ad2307644dcac603e1f1f0feb72]- | ||
| linters <- lintr::modify_defaults( | ||
| linters | ||
| , line_length_linter = NULL # we see how long lines are when we write them |
There was a problem hiding this comment.
Not a big fan of leading commas, but also not very important. :)
There was a problem hiding this comment.
It's a bit tongue-in-cheek :)
The default lintr config we have on dv.templates is just bad because it cares about formatting and deactivates useful linters such us object_usage_linter. However, having such annoying defaults forces the main developers of each repository to decide what is it that they care about, because one size does not fit all. And I see that as a good thing.
My formatting of this file is just an expression of how little I care about formatting. I won't add leading commas anywhere else and I won't complain if someone modifies these. And more importantly, I won't complain if the maintainers of these repository modify the set of linters they care about.
R/check_call_auto.R
Outdated
| flags <- list(one_or_more = TRUE) | ||
| OK[["dataset_names"]] <- CM$check_dataset_name("dataset_names", dataset_names, flags, datasets, used_dataset_names, | ||
| warn, err) | ||
| "TODO: default_vars (group)" |
There was a problem hiding this comment.
These TODOs will be done later?
There was a problem hiding this comment.
The TODOs (like the rest of the contents of this file) are autogenerated from the this spec, which is incomplete because the CM spec language doesn't have words to express the semantics of some of the parameters of this modules (those flagged as ignored).
The checks they should contain has been written by hand here, so I would consider that those TODOs are actually addressed. I just don't remove them because I prefer not to modify autogenerated files.
I haven't extended the API parameter types or the check generator (CM$generate_check_functions) to cover this scenario because I don't have another example that justifies that generalization. Instead I'm using the "escape hatch" of writing the checks by hand.
There's a much more elegant example of the use of this escape hatch in dv.edish, where all parameters can be expressed with the TC API types but the module still benefits from manual finetuning. Maybe I could take some time to talk about this in an upcoming devOH meeting...
There was a problem hiding this comment.
Wouldn't it make sense to mark somehow that the TODOs are actually already done?
So if you have a look at the code to a later time, that you know that this TODO ist not really a TODO.
There was a problem hiding this comment.
I see that the TODOs on the autogenerated code are no ideal. I'll replace them with NOTEs explaining that the expectation is that there will be code at the calling site taking care of the semantics of the parameters that are tagged as ignored.
| @@ -479,3 +440,79 @@ mod_listings <- function( | |||
| ) | |||
| return(mod) | |||
| } | |||
|
|
|||
| # Listings module interface description ---- | |||
| # TODO: Fill in for dressing room and automatic generation of docs | |||
There was a problem hiding this comment.
Dressing room wil also be added later, right?
There was a problem hiding this comment.
The scope of this PR (and the associated sprint goal) is the early error feedback, although we've smuggled the behavior of communicating the datasets we use to dv.manager and the removal of dispatchers. All of these features provide are valuable by themselves.
Adding dressing room support would require a non-trivial generalization of TC.R and of DR.R that we should plan for on upcoming sprints, if we decide to prioritize it.
Up until now the dressing room feature has only been discussed/used as a helper for the core davinci team and it's only public to app creators of dv.papo that care too look for it in the reference manual. Since the interface of this module is fairly straightforward, I see little immediate benefit in equipping it with dressing room support. HOWEVER, if we decided to encourage app creators to rely on the dressing room, then I think that complete coverage of our dv.* module ecosystem would be a more compelling proposition.
There was a problem hiding this comment.
Agreed, the module is so simple it doesn't benefit that much from a dressing room. We can revisit this once we equip the other modules with a dressing room.
The "early error feedback" feature is the most substantial, code-wise, but it just reuses code that is already working in other modules (e.g.
dv.explorer.parameter).We can do this PR review sync or async.
Thanks!
Critical checks
Is the version number correct?
DESCRIPTION file
NEWS.md
Does the build pass?
Documentation
Does it include the following sections?
Module introduction with features
Installation details
Explanation of function arguments
Data specifications and requirements
Different possible visualizations
Are the changes/new features included in NEWS.md?
(O) Screenshots
(O) Explanation of input menus
(O) Short articles on building the app, compatibility with other modules, known bugs,...
QC Report
Does it include a QC Report with positive outcome?
Are the new features reflected accordingly in the specs?
API conventions