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

CRAN release? #9

Open
krlmlr opened this issue Jul 23, 2020 · 14 comments
Open

CRAN release? #9

krlmlr opened this issue Jul 23, 2020 · 14 comments

Comments

@krlmlr
Copy link
Contributor

krlmlr commented Jul 23, 2020

Are you planning a CRAN release of this package any time soon?

@dirkschumacher
Copy link
Owner

Do you have a specific usecase? I would not mind putting on CRAN

@HenrikBengtsson
Copy link
Contributor

HenrikBengtsson commented Oct 14, 2021

I think I've got plenty of use cases for what you've solved quite elegantly here. Some examples:

  • optional run-time debugging (in the same spirit as debugme)
  • optional run-time logging, possibly at different amounts (e.g. INFO, DEBUG, TRACE)
  • optional run-time assertion, possibly partial or in full during checks (WISH: Near-zero overhead assertions r-lib/debugme#32)
  • optional run-time profiling (e.g. enable timing and memory profile of futures for troubleshooting purposes)
  • optional progress updates

... all with zero overhead when disabled (=removed on load). To me, as a developer trying to maximize correctness (=lots of assertions) and performance (=minimal number of asserts), being able to use macros that can be enabled and disabled when the package is loaded is of great value. Another benefit is that the code that print(<function>) outputs and that is used/seen when debugging is cleaner when above "non-essential" code is disabled/removed.

@HenrikBengtsson
Copy link
Contributor

HenrikBengtsson commented Oct 14, 2021

One wish, though, for a CRAN release, please minimize package dependencies to make this package as lightweight as ever possible.

Specifically, I think the only use for rlang is in:

result <- exec(fun, !!!as.list(ast)[-1])

I haven't tried, but that can probably be replaced by a do.call(fun, args = ...) call.

@dirkschumacher
Copy link
Owner

dirkschumacher commented Oct 14, 2021

I am open to doing that since there is obvious demand (at least 2 people :)). I agree that it would be good to have minimal dependencies to make the package as robust and maintainable as possible.

A quick list of things that need to get done for a first version:

  • Replace CI with Github Actions
  • Investigate removing rlang from Imports
  • Bonus: Replace the current imperative tree walker with a recursive one. This might cause problems with extremely deeply nested code, but comes with the advantage of easier maintenance and understanding for new contributors. Also, compiler is recursive, so I guess it should be fine.
  • Improve documentation (e.g. document usage #6)
  • Test the package in a real package :)
  • Maybe also document limitations. E.g. I could see problems when macros depend on the prior execution of other macros. This could be solved e.g. by restarting the expand onLoad function whenever a function is expanded.

Anything else?

This was referenced Oct 14, 2021
@dirkschumacher
Copy link
Owner

@krlmlr you added rlang, which made the code obviously more readable. Did it also solve any other issues?

@dirkschumacher
Copy link
Owner

@HenrikBengtsson before I publish it on CRAN, it would be great if you could test the package in one of your packages to see if it really works.

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 16, 2022

I don't mind removing rlang if it causes strong feelings.

Also, I'm ready to tick the "Test the package in a real package :)" bullet. Specifically, I'd like to explore usage of this package for expanding DBItest tests: r-dbi/DBItest#267. The linked PR exports way too many functions from DBItest. If those were macros, the tests would become larger, but also easier to troubleshoot in case of failures.

If a CRAN release in the near future is elusive, adding the package to r-universe would be great too: https://dirkschumacher.r-universe.dev/ui#packages.

@dirkschumacher
Copy link
Owner

dirkschumacher commented Oct 16, 2022

I don't mind removing rlang if it causes strong feelings.

It's already gone from the dependencies.

Also, I'm ready to tick the "Test the package in a real package :)" bullet.

Let's do it then. Only thing left is polishing the DESCRIPTION file I believe.

@dirkschumacher
Copy link
Owner

BTW. @HenrikBengtsson @krlmlr you both contributed to the package. Happy to add you to the DESCRIPTION file. Or better, make a PR :)

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 16, 2022

Thanks. I wonder why this package doesn't show up in your R universe. Do you need to set up manually?

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 16, 2022

I'll try to start using defmacro in DBItest in the next few days. I need to confirm that it actually solves my use case.

@dirkschumacher
Copy link
Owner

Sure go for it. Or do you need it on cran first?

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 16, 2022

No, rather the opposite -- I'd take the opportunity to provide feedback, if necessary, before it hits CRAN.

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 16, 2022

It wouldn't hurt if it were on R-universe, though.

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

3 participants