Skip to content

Raise TypeError for bad arguments to Mv#80

Merged
eric-wieser merged 3 commits intopygae:masterfrom
eric-wieser:strict-positional-mv-args
Nov 30, 2019
Merged

Raise TypeError for bad arguments to Mv#80
eric-wieser merged 3 commits intopygae:masterfrom
eric-wieser:strict-positional-mv-args

Conversation

@eric-wieser
Copy link
Member

If Mv is passed the wrong number of positional arguments, it used to either silently ignore them, or crash with an unclear message.

This would either confuse users, or mask bugs. With this change, it becomes a TypeError to pass either too many or too few parameters.

@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

Merging #80 into master will increase coverage by 0.16%.
The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   67.15%   67.32%   +0.16%     
==========================================
  Files           8        8              
  Lines        4808     4814       +6     
==========================================
+ Hits         3229     3241      +12     
+ Misses       1579     1573       -6
Impacted Files Coverage Δ
galgebra/mv.py 63.23% <91.3%> (+0.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9797860...c1458c9. Read the comment docs.

Comment on lines -251 to +259
def _make_odd(ga, *args, **kwargs):
def _make_odd(ga, __name_or_coeffs, **kwargs):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that unlike even, odd accepts not only a name, but coeffs of length l = 1 if ga.n % 2 == 1 else ga.n, with semantics "set the coeffiencients for the first l lexicographically sorted elements of each grade to coeffs". This is nonsense, but tracked in #81 to avoid scope creep.

@eric-wieser eric-wieser force-pushed the strict-positional-mv-args branch 3 times, most recently from 482199a to 7dc1c09 Compare November 27, 2019 12:47
@eric-wieser
Copy link
Member Author

Working on the CI failure on this one, the introduced issue is a fair callout

If `Mv` is passed the wrong number of positional arguments, it used to either silently ignore them, or crash with an unclear message.

This would either confuse users, or mask bugs. With this change, it becomes a `TypeError` to pass either too many or too few parameters.
@eric-wieser eric-wieser force-pushed the strict-positional-mv-args branch from 7dc1c09 to c1458c9 Compare November 29, 2019 15:19
@eric-wieser
Copy link
Member Author

Milestoning as 0.4.5 since the 2nd commit fixes a regression

@eric-wieser eric-wieser added this to the 0.4.5 milestone Nov 29, 2019
@eric-wieser
Copy link
Member Author

Actually, seems the CI failure isn't that easy to fix.

The complaint is the "Assignment Branch Condition" metric is too high in Mv.__init__. While this is maybe true, I don't think this change makes things significantly worse.

In generally, the function is just a mess because it has way too many overloads, which isn't something we can change anyway.

@utensil
Copy link
Member

utensil commented Nov 30, 2019

codebeat uses pending to indicate there's some style issue. First time it took me some time to figure out that it's not still checking. Bummer.

It's just a constant reminder for me. When I touch some code related to the code mess, it will remind me that I could do something about it in the current PR. But I'll just snooze it until next time.

But this time, I'll file an issue for it and merge this PR.

@utensil
Copy link
Member

utensil commented Nov 30, 2019

I've reviewed the code and consider it in a good status to merge. But since you haven't requested a review, I don't know if you have finished this work, if yes, please just go ahead and merge. I noticed that #90 is waiting on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: core Ga, Mv, Metric, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants