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

add Record.get default value #489

Merged
merged 4 commits into from Jan 9, 2020
Merged

add Record.get default value #489

merged 4 commits into from Jan 9, 2020

Conversation

@MykolaGolubyev
Copy link
Collaborator

MykolaGolubyev commented Jan 8, 2020

No description provided.

@MykolaGolubyev MykolaGolubyev requested a review from tsiq-karold Jan 8, 2020
void "should return defaultValue if a columnName is not present"() {
def record = new Record(new Header(["c1", "c2", "c3"].stream()), ["v1", "v2", "v3"].stream())
def valueC1 = record.get("c1", 42)
def valueC4 = record.get("c4", 42)

This comment has been minimized.

Copy link
@tsiq-clemens

tsiq-clemens Jan 8, 2020

Collaborator

or get("noSuchColumn", 42)?

This comment has been minimized.

Copy link
@MykolaGolubyev

MykolaGolubyev Jan 8, 2020

Author Collaborator

Intention here is to show more like columns that can be there but not in this case. I.e optional table columns. But I see what you mean. Sometimes I do that.

void "should return defaultValue if an idx is not present"() {
def record = new Record(new Header(["c1", "c2", "c3"].stream()), ["v1", "v2", "v3"].stream())
def valueC2 = record.get(1, 42)
def valueCN = record.get(5, 42)

This comment has been minimized.

Copy link
@tsiq-clemens

tsiq-clemens Jan 8, 2020

Collaborator

why 5? 3 is the edge case. I'd also indicate the too-high-index in the variable name

This comment has been minimized.

Copy link
@MykolaGolubyev

MykolaGolubyev Jan 8, 2020

Author Collaborator

5 cause more than 2 :)
I feel like the scope of this test is to small to introduce named vars. If you feel strongly about it, I will change.

This comment has been minimized.

Copy link
@MykolaGolubyev

MykolaGolubyev Jan 8, 2020

Author Collaborator

Ok, changed 5 to 3 :)

Copy link
Collaborator

tsiq-clemens left a comment

LGTM

# Conflicts:
#	webtau-core/src/main/java/com/twosigma/webtau/data/table/Record.java
@MykolaGolubyev MykolaGolubyev merged commit e558777 into master Jan 9, 2020
4 checks passed
4 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@MykolaGolubyev MykolaGolubyev deleted the record-default-value branch Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.