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

rationalize function names and argument-names #15

Closed
ijlyttle opened this issue Jul 10, 2018 · 9 comments
Closed

rationalize function names and argument-names #15

ijlyttle opened this issue Jul 10, 2018 · 9 comments

Comments

@ijlyttle
Copy link
Contributor

it would be good to have standard definitions (and names) of function arguments

this will involve judicious use of the @inheritParams tag

@ijlyttle ijlyttle changed the title rationalize arguments rationalize function names and argument-names Jul 24, 2018
ijlyttle added a commit that referenced this issue Jul 24, 2018
ijlyttle added a commit that referenced this issue Jul 24, 2018
now that we have the floater at the top-right (defaultStyle = TRUE), less need for action links
@ijlyttle
Copy link
Contributor Author

ijlyttle commented Jul 24, 2018

Given @AliciaSchep's suggestion, this seems the place to propose a new function-naming scheme.

Current view - following @AliciaSchep's proposal below:

Done staring - will start implementing (input still welcome!)

Non vw_ namespace

I think these functions should be exempted from the vw_ scheme because they have roots in other places, serve as the gateway to the package, and (I think) have little chance of colliding (I need to find a better way to express this idea):

as_vegaspec()
vegawidget()
vegawidgetOutput()
renderVegawidget()
vega_embed()
use_vegawidget() - not done yet, but a usethis-style templating function for other packages (e.g. altair) to import and re-export vegawidget functions, like usethis::use_pipe().

vw_ namespace

Functions

vw_as_json() - returns JSON, but still coercible to vegaspec
vw_autosize()
vw_block_config() - returns YAML string for block-configuration
vw_create_block()
vw_create_block_gistid() - returns list with gist information
vw_retrieve_block()
vw_examine()
vw_spec_version() - returns list of spec information
vw_png_bin() - returns binary from base64-encoded DataURI
vw_to_png() - returns PNG base64-encoded DataURI
vw_to_svg() - returns SVG string
vw_to_vega()
vw_write_png() - returns what was sent to it (vegaspec or vegawidget)
vw_write_svg() - returns what was sent to it (vegaspec or vegawidget)

Data

vw_ex_mtcars

I am not completely happy with the name spec_mtcars (or vw_spec_mtcars). I want something concise, evocative, and different enough from other names - as well as extensible to other sample specs that we might add in the future. However, each time I move away from spec_mtcars, I keep coming back to it. vw_ex_mtcars has possibilities, though: the ex_ can denote "example", and it costs just one more character than spec_mtcars. Open to ideas here, too!

@AliciaSchep
Copy link
Contributor

AliciaSchep commented Jul 24, 2018

I think for vega_embed it makes sense to omit the vw_, partially because it has vega in the name (like all the other exempted functions in list above). I would lean towards including vw for block_config so that the name is tied to this package specifically (even if no package currently has a similar function, other widget packages might in future?). Then it also helps keep the rule of thumb that functions in package all either have 'vega' in name or are prefixed by 'vw_'....

ijlyttle added a commit that referenced this issue Jul 24, 2018
standardizes on "vega_lite" as representation of Vega-Lite
@ijlyttle
Copy link
Contributor Author

ijlyttle commented Jul 24, 2018

Thanks!

Sorry for not noticing this earlier, I must have been too "busy" wrapping my head around the ideas and tweaking them. I think we are essential agreement (thanks for your patience); I just need to settle block_config() and png_bin() in my head.

Let me adjust the proposal above to what you suggest, and I will stare some more...

On the one hand, I'm likely thinking about this too much - on the other hand, as you know, this is the best chance to make the changes.

@ijlyttle
Copy link
Contributor Author

@AliciaSchep, having had a chance to stare at it for a bit, I like this!

I had a scheme where vs_ would imply that the function took a vegaspec, but I think it ran the risk of being too-fragile. Your proposal of using vw_ and linking it to the package name seems much more robust. Thanks again!

These remaining points might be fine as they are, but they both seem a little unsettled in my eyes.

  • name for example specs, e.g. vw_ex_mtcars (vw_mtcars?)
  • name for vw_png_bin(), which takes the dataURI string from vw_to_png() and turns it into a raw vector.

We need vw_png_bin() internally - if it's not exported, then we don't care. But it seems useful enough to export in case someone wants the binary rather than the dataURI string.

Thanks again - of course, any input you have on anything is most welcome!

@ijlyttle
Copy link
Contributor Author

Just to note that, non-withstanding new proposals on example-spec naming or vw_png_bin(), I will begin implementing this.

@AliciaSchep, thanks!

ijlyttle added a commit that referenced this issue Jul 25, 2018
ijlyttle added a commit that referenced this issue Jul 25, 2018
ijlyttle added a commit that referenced this issue Jul 25, 2018
ijlyttle added a commit that referenced this issue Jul 25, 2018
ijlyttle added a commit that referenced this issue Jul 25, 2018
ijlyttle added a commit that referenced this issue Jul 25, 2018
@ijlyttle
Copy link
Contributor Author

I think I have things switched over now - I'll leave this open for a while in case we need to adjust anything.

@AliciaSchep
Copy link
Contributor

Looks good! Re the example spec, I think having spec in the name is nice. vw_mtcars_spec, mtcars_spec, spec_mtcars, vw_spec_mtcars, etc. I think other packages that use the prefix convention sometimes exempt data sets from that convention.

@ijlyttle
Copy link
Contributor Author

Ha! This is perfect! I mean this absolutely earnestly that I had a good laugh when, upon taking your response into consideration, I realized spec_mtcars revealed itself to still be "it".

I like its conciseness, that spec_ denotes that it is data, and seems namespace-safe.

I'll change it back...

@ijlyttle
Copy link
Contributor Author

Reverted in 71b64c1

Closing this, as I think everything is ironed out and implemented 🎉

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

No branches or pull requests

2 participants