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

test with new paradox #43

Merged
merged 11 commits into from
Mar 17, 2024
Merged

test with new paradox #43

merged 11 commits into from
Mar 17, 2024

Conversation

mb706
Copy link
Contributor

@mb706 mb706 commented Jan 14, 2024

No description provided.

@mb706 mb706 mentioned this pull request Jan 14, 2024
@m-muecke
Copy link
Member

m-muecke commented Feb 3, 2024

Ah haven't seen your PR, I have made the same changes, but refactored a bit more would recommend to go for that PR instead: #49

@mb706 mb706 marked this pull request as ready for review February 28, 2024 15:36
@mb706
Copy link
Contributor Author

mb706 commented Feb 28, 2024

@damirpolat this PR prepares for the upcoming paradox release, which can already be seen on github: mlr-org/paradox. The new version will throw an error when a parameter has a 'default' value and also has the 'required' tag. This is because the semantics of the 'default' value is "what does this parameter behave like when it is not set" -- this is in contradiction with the "required" tag, which indicates that a parameter must always be set.
Besides that, there is also a new workflow which checks against the github version of paradox. This is because paradox on github will not be merged for a few more weeks (until all the other changes are on CRAN and other package maintainers had their time to follow suit). Until then, it is best to check both against CRAN as well as github versions of various packages.

@mb706
Copy link
Contributor Author

mb706 commented Feb 28, 2024

Please merge this PR if you are fine with it.

@mb706
Copy link
Contributor Author

mb706 commented Feb 28, 2024

(I think you should also put it on CRAN soon, so paradox can then be updated without causing revdep failures)

@m-muecke
Copy link
Member

@mb706 why did you remove the crate function for the custom checks, does it not have an effect here?

@mb706
Copy link
Contributor Author

mb706 commented Feb 28, 2024

The crate()s are not necessary for functions that are defined in the package. We do crate() for anonymous custom_check functions that are defined in-place to remove their environment -- otherwise the resulting objects keep the environment of the initialize() call around. That is not necessary here.

@m-muecke
Copy link
Member

The crate()s are not necessary for functions that are defined in the package. We do crate() for anonymous custom_check functions that are defined in-place to remove their environment -- otherwise the resulting objects keep the environment of the initialize() call around. That is not necessary here.

That means custom_check = crate(function(x) check_numeric(x))) can be changed to custom_check = check_numeric and only required for the inplace anonymous function?

@mb706
Copy link
Contributor Author

mb706 commented Feb 29, 2024

Yes, putting checkmate check_-functions inside anonymous functions is only necessary if you want to combine different checks, or if you want to modify the error message. But if you do use anonymous functions, crate() should be used.

@mb706
Copy link
Contributor Author

mb706 commented Mar 8, 2024

@damirpolat I am done here, you resolve the conflicts, merge andput the package on cran.

@damirpolat damirpolat merged commit eedf186 into main Mar 17, 2024
4 checks passed
@damirpolat
Copy link
Member

Thank you for your pull request! It is merged now.

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