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

Implement #1008 #1009

Closed
wants to merge 1 commit into from
Closed

Implement #1008 #1009

wants to merge 1 commit into from

Conversation

ilyaZar
Copy link
Contributor

@ilyaZar ilyaZar commented Mar 24, 2023

This PR could be used to resolve #1008 . The last bullet of the commit message is probably subject to change, also the name "class" could be altered.

The boilerplate is not minimal/empty, but one almost always uses a public interface when creating a R6 class so it may be fine if it is already there

Otherwise, works exactly the same as add_fct() and add_utils().

@ColinFay
Copy link
Member

ColinFay commented Apr 4, 2023

Hey @ilyaZar

Thanks for your work. I won't merge for now, I need to think about this a little bit but overall I think it's a great idea :)

@ilyaZar
Copy link
Contributor Author

ilyaZar commented Apr 4, 2023

sure! thanks for taking time to have a look at this @ColinFay !

Works exactly the same as add_fct() and add_utils().

- add argument 'r6' to add_module()
- add add_r6() helper function calling add_r_files with slightly different parameters
- adjust append_roxygen_comment() to create R6 template
- add man files and NAMESPACE exports
@VincentGuyader
Copy link
Member

It's certain that when referring to a file, we can't use pkg and keep path. However, the meaning behind the pkg parameter isn't very clear to me. I would tend to think that it expects a character string that references a package name.

For example, this seems strange to me: pkg = get_golem_wd().

Why not use pkg_dir, or simply golem_wd when referring to the project that contains the Golem, for instance?

In any case, the deprecation of the parameter will need to be managed, with a message for users (similar to what was done in readr for transitioning from path to file).

@ilyaZar
Copy link
Contributor Author

ilyaZar commented Aug 9, 2023

@VincentGuyader are you referring to #1048 ?

@ColinFay
Copy link
Member

Will cherry pick this one in a new branch and update :) will let you know :)

@ColinFay
Copy link
Member

Thanks, cherry picked here #1118

@ColinFay ColinFay closed this Nov 17, 2023
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

Successfully merging this pull request may close these issues.

3 participants