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

PhysicalTable uses getAvailableIntervals #79

Merged
merged 1 commit into from
Oct 28, 2016

Conversation

archolewa
Copy link
Contributor

--PhysicalTable::getAvailableIntervals returns
availableIntervalsRef.get(). However, the other methods didn't actually
use getAvailableIntervals internally. Instead, they used
availableIntervalsRef.get() directly. As a result, overriding
getAvailableIntervals would not actually be sufficient to restrict a
PhysicalTable's view of its availability, despite the method name.

--Now, all invocations of availableIntervalsRef.get() have been
replaced with getAvailableIntervals, making it much easier to enforce
a consistent view of availability simply by overriding
getAvailableIntervals.

@cdeszaq
Copy link
Collaborator

cdeszaq commented Oct 27, 2016

👍 (though a changelog entry would be nice, since this could break someone if they were relying on internal things not calling that getter)

@archolewa
Copy link
Contributor Author

archolewa commented Oct 27, 2016

@cdeszaq The only way I can think of that happening would be if someone is overriding and changing getAvailableIntervals not to affect Fili's behavior (since we don't actually use it anywhere but tests), but solely for use in their own code, which would be a whole new level of weird.

But I've added a changelog entry anyway, because whatever.

@michael-mclawhorn
Copy link
Contributor

👍
I would actually regard this as safe not to CL, although this is a perfect candidate for establishing guidance around what is and isn't a public api.

@cdeszaq
Copy link
Collaborator

cdeszaq commented Oct 28, 2016

My 1st thought was that this was below the level of noting in the CL, but since PhysicalTable is one of the Core Domains within the system, and it seems that the thrust of the PR is to make it easier to extend, it made sense to err on the side of being overly communicative in the CL about it than surprise possible extenders.

--`PhysicalTable::getAvailableIntervals` returns
`availableIntervalsRef.get()`. However, the other methods didn't actually
use `getAvailableIntervals` internally. Instead, they used
`availableIntervalsRef.get()` directly. As a result, overriding
`getAvailableIntervals` would not actually be sufficient to restrict a
`PhysicalTable`'s view of its availability, despite the method name.

--Now, all invocations of `availableIntervalsRef.get()` have been
replaced with `getAvailableIntervals`, making it much easier to enforce
a consistent view of availability simply by overriding
`getAvailableIntervals`.
@archolewa archolewa force-pushed the physical-table-uses-getAvailability branch from 96a2e59 to 9ccb775 Compare October 28, 2016 18:14
@archolewa archolewa merged commit b1ef7a2 into master Oct 28, 2016
@archolewa archolewa deleted the physical-table-uses-getAvailability branch October 28, 2016 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants