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

Changed DataSet.merge signature #6424

Merged
merged 2 commits into from Sep 14, 2018

Conversation

@HGuillemet
Copy link

commented Sep 12, 2018

This small PR addresses an immediate problem I had : getRange returns an api.DataSet, which is logical since getRange is inherited from api.DataSet interface. But DataSet.merge only takes a list of DataSet.
So no way to merge a dataset obtained from getRange without cast.

However, there is a deeper design problem : api.DataSet references DataSet, which ruins the point of having a separate interface. I think one of the two following actions must be taken:

  1. Replace DataSet with api.DataSet anywhere in api.DataSet, and in signatures of methods in DataSet implementing an interface method. There will be an issue with the whole interface currently implementing Iterable. Maybe this must be removed. Implementing Iterable<? extends api.DataSet> is not possible. implementing Iterable<api.DataSet> would not be very consistent. Renaming DataSet with something like MemoryDataSet or some name that evokes what is specific to this class WRT other possible implementations would be good too and less confusing.
  2. Decide that an api.DataSet is always implemented by DataSet (which is necessarily the case right now) or a subclass and merge them.

These remarks about the api/implementation split hold for DataSet, but may hold for others, I didn't check.

@AlexDBlack
Copy link
Contributor

left a comment

LGTM, thanks 👍
And apologies for the delay getting to this, I ended up having to build and test this manually/locally (CI wasn't being triggered properly on it)

@AlexDBlack AlexDBlack merged commit 003a573 into eclipse:master Sep 14, 2018

0 of 2 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
continuous-integration/jenkins/pr-head This commit cannot be built
Details
@saudet

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

@sshepel CI seems down or is this an issue only with pull requests from forks?

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

In this case it didn't seem to trigger any builds, like there were no changes - I haven't noticed that on any of my PRs, so maybe it's limited to PRs from forks...

@HGuillemet HGuillemet deleted the HGuillemet:hg_dataset_interface_cleanup branch Sep 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.