Rename and document fields in Access.#24778
Conversation
andriyDev
added
C-Docs
Zeophlite
left a comment
Zeophlite
left a comment
There was a problem hiding this comment.
Confirmed this is just renames
chescock
left a comment
chescock
left a comment
There was a problem hiding this comment.
Looks good! I think it needs a migration guide before merging, though.
Did we want to consider renaming add_write to add_read_and_write and remove_read to remove_read_and_write? That could also help for clarity, since add_write() now affects the value returned by try_reads(). But it's also a lot more verbose.
| /// Returns the set of components with read or write access, | ||
| /// Returns the set of components with read access, | ||
| /// or an error if the access is unbounded. | ||
| pub fn try_reads_and_writes(&self) -> Result<&ComponentIdSet, UnboundedAccessError> { |
There was a problem hiding this comment.
I think we need a migration guide for this, and it might be good to leave the old name as #[deprecated] for a release.
| /// present in `Self::read_and_writes`. | ||
| read_and_writes_inverted: bool, | ||
| /// present in [`Self::reads`]. | ||
| read_inverted: bool, |
There was a problem hiding this comment.
This is pluralized in writes_inverted, so we should probably be consistent. But if we're going to replace it with an enum soon then it doesn't really matter!
| read_inverted: bool, | |
| reads_inverted: bool, |
kfc35
added
S-Ready-For-Final-Review
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
andriyDev
left a comment
andriyDev
left a comment
There was a problem hiding this comment.
@chescock Personally, I don't think we need the extra verbosity. To me at least, it seems intuitive that write access implies read access - that's exactly how system params work, if I ask for &mut Component I also get read access (and the doc comments also spell this out now).
The one I'm least confident about is remove_read_and_write. I think though just changing the doc comment is sufficient.
| /// present in `Self::read_and_writes`. | ||
| read_and_writes_inverted: bool, | ||
| /// present in [`Self::reads`]. | ||
| read_inverted: bool, |
| /// Returns the set of components with read or write access, | ||
| /// Returns the set of components with read access, | ||
| /// or an error if the access is unbounded. | ||
| pub fn try_reads_and_writes(&self) -> Result<&ComponentIdSet, UnboundedAccessError> { |
andriyDev
force-pushed
the
rename-reads-and-writes
branch
from
54856c7 to
943c5e1
Compare
andriyDev
added
S-Ready-For-Final-Review
4 participants
Objective
Access.Solution
reads_and_writesto justreads, and explain in the doc comment that write access also implies read access.