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

let indicator allow strings #2284

Merged
merged 6 commits into from
Jun 18, 2020
Merged

let indicator allow strings #2284

merged 6 commits into from
Jun 18, 2020

Conversation

pdeffebach
Copy link
Contributor

Fixed #2283

@bkamins
Copy link
Member

bkamins commented Jun 11, 2020

Apart from the small comments there is a small problem.
You have left out a test:

Symbol(indicator_cols[i]) == indicator

which will always fail is indicator is a string.

However, now as I look at it it actually seems it can be removed if we move line:

select!(joined, Not([df1_ind, df2_ind]))

before line:

unique_indicator = indicator

(if it is not clear for you why then I can fix this after this PR is merged)

@bkamins bkamins added backport non-breaking The proposed change is not breaking labels Jun 11, 2020
@bkamins bkamins added this to the 1.0 milestone Jun 11, 2020
@pdeffebach
Copy link
Contributor Author

Thanks for the review.

Indeed I cannot figure out this error you described. I can't figure out a way to write code that throws an error about comparing a symbol to a string. I think you may have to fix it after the merge.

@bkamins
Copy link
Member

bkamins commented Jun 17, 2020

OK - I can clean up the code later.

For now can you please just add after line https://github.com/JuliaData/DataFrames.jl/pull/2284/files#diff-3d9293bcc4eac4333160c4d203ae91d7R244 (i.e. as a first statement executed if indicator !== nothing is true)
the following line:

indicator = Symbol(indicator)

it will fix the bug (not in the cleanest way, but will do for now) as then we are sure that in Symbol(indicator_cols[i]) == indicator) we have symbols both on left and right hand sides.

I cannot fix it via web interface unfortunately.

@pdeffebach
Copy link
Contributor Author

Added! let me know what else to add

@bkamins
Copy link
Member

bkamins commented Jun 18, 2020

Looks good. Thank you!

@bkamins bkamins merged commit dac42dd into JuliaData:master Jun 18, 2020
@bkamins
Copy link
Member

bkamins commented Jun 18, 2020

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport non-breaking The proposed change is not breaking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

indicator column in joins should allow Strings
2 participants