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

improve assert_class(). closes #770 #772

Merged
merged 2 commits into from
Sep 19, 2023
Merged

improve assert_class(). closes #770 #772

merged 2 commits into from
Sep 19, 2023

Conversation

JanMarvin
Copy link
Owner

No description provided.

@olivroy
Copy link
Collaborator

olivroy commented Aug 25, 2023

That's good. and about that, that's why I added arg_nm when I was developping wb_dims() in #702

assert_class is not functioning entirely as expected.

wb_comment(author = 12)
#> Error x must be class character.

It would be better if it was author must be class character.
It would be fixed by adding arg_nm = "author" in the assert code.
I wouldn't mind correcting that eventually but if that's an easy fix, feel free to look into it,

@JanMarvin
Copy link
Owner Author

Thanks, yes I had a quick look at this, while preparing the release and figured something like this must have been the reason for arg_nm. I wasn't entirely sure if this would have side effects therefore it did not make the cut. All of the asserts were written by Jordan. I can have a look if there's some other way to simply tackle this. But first I'm gonna get some rest and afterwards write the release note 😴

@olivroy
Copy link
Collaborator

olivroy commented Aug 25, 2023

Indeed, it's not pressant. My next rounds of improvements will be regarding error messages. But possibly in a month or so!

Just wanted to highlight it here since it seemed related.

@olivroy
Copy link
Collaborator

olivroy commented Sep 1, 2023

‘Object not found’ and ‘Missing argument’ errors now give a more accurate error context. Patch provided by Lionel Henry in PR#18241.

I don't know how this looks like on R 4.3 vs R 4.2 (still using R 4.2, but maybe base R's patch is enough)

I just noticed that, and thought it was relevant, but possibly not. Please hide the comment if this is not relevant.

@JanMarvin JanMarvin merged commit b078e9c into main Sep 19, 2023
9 checks passed
@JanMarvin JanMarvin deleted the gh_issue_770 branch September 19, 2023 18:12
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.

2 participants