WIP Implement fixed functionality in all designers

DeclareDesign / DesignLibrary

Library of Research Designs

https://declaredesign.org/library/

Other

30 stars 3 forks source link

WIP Implement fixed functionality in all designers #233

Closed clarabicalho closed 5 years ago

clarabicalho commented 5 years ago

Description

Implements fixed functionality in all designers using combination of glue::glue() and substitute() approaches. Note:

This approach does not affect the source code of any designers. Arguments passed onto args_to_fix are replaced for their values inside the construct_design_code() function.
As it is now, all assignments in the body of the designer function need to be separated by new line, as a way of flagging unique expressions in the code string vector. Any suggestions for how to remove this restriction are welcome.
Closes #231

Checklist

[X] Branch is passing devtools::check() with 0 errors, warnings, or notes

jaspercooper commented 5 years ago

Wow, this looks pretty amazing. And really really nice that you don't have to change the source code into rlang salad. Thanks for the work on this!

@nfultz would be great to get your take on this.

Small thing that has been bugging me: the argument "fixed" doesn't seem expressive enough, and has connotations with other things (effects, issues, breakages). What we're trying to say is "these are the arguments will not be output as variables in the code, but will be fixed at the value provided when the design is declared," right?

So I think we could have something like "fixed_arguments" or "constants." @macartan any thoughts?

@clabima would you mind updating news.md with a description of the work in this PR? don't want to misrepresent it in the eventual release.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 1356

96 of 96 (100.0%) changed or added relevant lines in 15 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.004%) to 99.889%

Totals
Change from base Build 1331:	0.004%
Covered Lines:	896
Relevant Lines:	897

💛 - Coveralls

jaspercooper commented 5 years ago

@nfultz thanks so much for this thorough review and for the suggestions

clarabicalho commented 5 years ago

Indeed! Working on them now.

Em qui, 28 de fev de 2019 às 12:45, Jasper Cooper notifications@github.com escreveu:

@nfultz https://github.com/nfultz thanks so much for this thorough review and for the suggestions

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DeclareDesign/DesignLibrary/pull/233#issuecomment-468243298, or mute the thread https://github.com/notifications/unsubscribe-auth/AYa0JN-aPaTmuq39P3kO4PYGm32ucR7vks5vR8FMgaJpZM4bVo3p .

jaspercooper commented 5 years ago

Thanks for these updates @clabima !

clarabicalho commented 5 years ago

Sure! I think just missing news.md update and addressing the coverage issue. Will push these asap.

clarabicalho commented 5 years ago

Should be ready to merge when checks go through. @jaspercooper @nfultz thank you for all the helpful feedback.

jaspercooper commented 5 years ago

Amazing work, thanks so much @clabima! and thanks to you @nfultz for the careful review and good ideas.