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

Always return a RegionCollection from #subregions #177

Merged
merged 1 commit into from
May 26, 2015

Conversation

codeodor
Copy link
Contributor

Prior to this, #load_subregions returned an empty array if there were no subregions. But there is no reason the calling code should have to check the return type before operating on the return value.

Prior to this, #load_subregions returned an empty array if there were no subregions. But there is no reason the calling code should have to check the return type before operating on the return value.
@ecbypi
Copy link
Contributor

ecbypi commented May 24, 2015

Looks good. @jim, @cdainmiller any thoughts?

@codeodor, what are your thoughts on adding tests to ensure an empty RegionCollection behaves as expected?

@codeodor
Copy link
Contributor Author

Since RegionCollection derives from array, unless Carmen is doing something to violate Liskov Substitution Principle (which I would doubt), I think it's safe.

That said, it's also a self-serving assessment because I'm swamped and only made this change because I figured it was probably a 1-liner that I could find in just a minute. 😄

I'll see what I can cook up if I get a few minutes later today.

@ecbypi
Copy link
Contributor

ecbypi commented May 24, 2015

Since RegionCollection derives from array, unless Carmen is doing something to violate Liskov Substitution Principle (which I would doubt), I think it's safe.

Sounds good. Just want to see if @jim or @cdainmiller have any thoughts before merging.

@jim
Copy link
Collaborator

jim commented May 26, 2015

Looks good to me.

FWIW, Having RegionCollection subclass Array was probably a mistake on my part. It would have been better to just include Enumerable and delegate each to an internal Array.

ecbypi added a commit that referenced this pull request May 26, 2015
Always return a RegionCollection from #subregions
@ecbypi ecbypi merged commit f96213e into carmen-ruby:master May 26, 2015
@codeodor
Copy link
Contributor Author

Awesome, thanks! 👍

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