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

Throw more specific error, ArgumentError, rather than a general Error() #1872

Merged
merged 7 commits into from
Sep 7, 2019

Conversation

petershintech
Copy link
Contributor

Also changed the unit test to check ArgumentError, not ErrorException.
#1867

src/dataframe/dataframe.jl Outdated Show resolved Hide resolved
Co-Authored-By: Bogumił Kamiński <bkamins@sgh.waw.pl>
@petershintech
Copy link
Contributor Author

@bkamins Thanks. I agree your code is easy to read. In fact, I can see several long lines exploiting short circuits. Do you think we need to rewrite the lines to more readable ones? If so, I can help you.

@bkamins
Copy link
Member

bkamins commented Jul 5, 2019

I can see several long lines exploiting short circuits.

Sure - thank you. The "soft" rule is that lines should not be longer than 92 characters.

petershintech and others added 2 commits July 5, 2019 21:46
Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>
@petershintech
Copy link
Contributor Author

I like the new message about more specific about the cause. Commit to the branch and ready to be merged. Thanks.

@petershintech
Copy link
Contributor Author

One quick question before merging. In this case,
if !(size(df, 1) == length(item) || size(df, 2) == 0)
should I throw ArgumentError or DimensionMismatch? I think the latter one is more specific about error cause so it would be better than the first one. What do you think of it?

@nalimilan
Copy link
Member

Right, DimensionMismatch sounds better.

@petershintech
Copy link
Contributor Author

Ok. I have replaced ArgumentError with DimensionMismatch and also touched a unit test for it. I can see the same change would be desirable for some other code lines but it can be captured in a separate PR. I think it is ready to merge. Thanks for all your inputs.

@bkamins
Copy link
Member

bkamins commented Jul 5, 2019

Yes, we used ArgumentError or BoundsError only everywhere up till now. We can be more specific of course 😄. Thank you.

@nalimilan nalimilan merged commit 1e288a7 into JuliaData:master Sep 7, 2019
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.

None yet

3 participants