moodymudskipper / flow

View and Browse Code Using Flow Diagrams
https://moodymudskipper.github.io/flow/
Other
395 stars 26 forks source link

breaking changes #56

Closed moodymudskipper closed 3 years ago

moodymudskipper commented 3 years ago

confusing ellipsis

as arguments pile up, having ... forwarded to nomnoml doesn't look good, and it's not that flexible either.

An argument engine.opts expecting a list of parameters, which could default to getOptions("flow.engine.opts") so these could be set globally too.

If the list has names "plantuml" and "nomnoml" we'll take the right item depending on the engine param (so user can set global options that work for all engine). If not we'll feed the list as is to the relevant engine function.

clunky range argument

We might remove it altogether, and add later if needed. It's only useful for huge functions for which the diagram engines are the limitation. In practice that's very few.

width and height

They're forwarded to hmlwidget but it's not clear how they work with flow.

probably better either combine them in a list of args to forward to htmlwidget, orjust remove altogether

sub_fun_id

now that we allow names and not only ids, the name is not great, if we modify the api maybe take the opportunity to change this argument name too ? sub_function ? sub_fun ? nested_fun ?


Another good consequence of these changes is that it would harmonize the api for nomnoml and plantuml as our list of args wouldn't be cluttered by args that apply only yo nomnoml. They'd be in the end as nomnoml.args and htmlwidgets.args, and maybe styler.args (but maybe better not to add too much flexibility upfront).


I think we might unexport flow_data and build_nomnoml data too. The users don't really care at this point and it's confusing re plantuml vs nomnoml.

moodymudskipper commented 3 years ago

current opinion with a fresh look after a few months, agree mostly with above :

github-actions[bot] commented 2 years ago

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.