Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
211
[.teal_data
S3 method #346base: main
Are you sure you want to change the base?
211
[.teal_data
S3 method #346Changes from 5 commits
967c1c5
362b982
a2610a3
c01612f
fd4fe27
f7894e5
e53afc8
1ea9d8f
220e46c
5f17af0
6d3c3e8
39d9f37
b6c6955
009bbc0
fc0d0c0
f74635a
d3eb395
5839f0d
269b77b
1c2f05a
5b1e1a8
7fd89a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this is inherited from
[.qenv
but usually subset usesi
argument to reuse the generic base extractor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but with
i
it works fornumeric
input, and we only supportcharacter
input :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then in my opinion, the natural way to handle this would be to convert the numeric input to names and use that internally, easier for the user and simple for the developer. But it can also provide an informative error message to users. Users will try to use numbers given that this is what usually R accepts. Sorry, to raise a new thing, could you add a test for numeric subsetting and NA_character_? I know we get the Assertion error, but I want to make sure this is tested/documented for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@llrs-roche oh yeah, but we don't want to introduce anything for numeric as
qenv
is basically anenvironment
that is not subsettable for numeric as it does not have a natural order (like atomic vectors have).Numbers are fine for atomic vectors, or simple S3 objects that naturally use indexes, but I don't think it is the case for the environmemnt. We actually don't want it specifically as we only allow extraction by name, not by numeric index, so that you specifically need to provide the name of the object. With numeric input you can get a different element, depending on the current state of the object : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, good idea : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are extended tests 5f17af0
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.