#29083 closed enhancement (fixed)

first parameter of a normal method must be self

Reported by: chapoton Owned by:
Priority: major Milestone: sage-9.1
Component: refactoring Keywords:
Cc: tscrim, jmantysalo Merged in:
Authors: Frédéric Chapoton Reviewers: Jori Mäntysalo
Report Upstream: N/A Work issues:
Branch: 0620c10 (Commits, GitHub, GitLab) Commit: 0620c10164128419db1e1a163a600937dcd6184b
Dependencies: Stopgaps:

Status badges

Description

as suggested by lgtm

Change History (23)

comment:1 Changed 22 months ago by chapoton

  • Branch set to u/chapoton/29083
  • Commit set to 7447f1f3914c1e9c332f5c96ccb5a556c561bd8f
  • Status changed from new to needs_review

New commits:

7447f1ffirst parameter must be self

comment:2 Changed 22 months ago by chapoton

  • Cc tscrim jmantysalo added

bot is morally green. Rather big diff this time, sorry. Please review.

comment:3 Changed 22 months ago by jmantysalo

  • Status changed from needs_review to needs_work

Nope. Just in the first function there is

J -- a fractional quaternion ideal with same order as I

but now I has changed to self.

comment:4 Changed 22 months ago by git

  • Commit changed from 7447f1f3914c1e9c332f5c96ccb5a556c561bd8f to 4d05dfb9caefd8e52e119713d1de81a03221534c

Branch pushed to git repo; I updated commit sha1. New commits:

4d05dfbtrac 29083 minor fixes

comment:5 Changed 22 months ago by chapoton

  • Status changed from needs_work to needs_review

ok, thanks, fixed. I also fixed another similar glitch and some pyflakes warning.

comment:6 Changed 22 months ago by jmantysalo

  • Reviewers set to Jori Mäntysalo
  • Status changed from needs_review to needs_work

A typo "Return the sum of selfy", otherwise this could be marked as positive review. However, I guess the error message "self and J must be right ideals" should be "- - of the same order" or something like that. But that's not your fault.

comment:7 Changed 22 months ago by git

  • Commit changed from 4d05dfb9caefd8e52e119713d1de81a03221534c to b21e31c5e77ed96c4854e31306ddec73f3f55955

Branch pushed to git repo; I updated commit sha1. New commits:

b21e31cfix typo in 29083

comment:8 Changed 22 months ago by chapoton

  • Status changed from needs_work to needs_review

Thanks a lot, Jori. I have fixed the typo, and left the error message as it is, not being able to change it into something better.

comment:9 Changed 22 months ago by jmantysalo

  • Status changed from needs_review to positive_review

Good now. Thanks.

comment:10 Changed 22 months ago by nbruin

  • Status changed from positive_review to needs_info

This ticket is creating a lot of churn in the revision history of files, but the benefit of the change is not argued. Is it really the case that functions in classes should have their first argument named "self"? It's indeed the case that on instances, these functions will be wrapped in bound methods that have their first argument as "self", but that doesn't necessarily say that the intent of the routine is best expressed by naming that parameter "self".

It may well be the case that the unbound functions do get called as well, in which case there isn't an obvious "self".

For instance, if you look in sage/structure/element.pyx, you can find plenty of routines that quite consistently name their arguments "left" and "right" (for coercion operations). In the style of __rmul__, it could well be that those arguments are swapped in order!

It could well be that the changes in this ticket have taken into account the subtleties that can arise and has only changed cases that benefit from using "self", but currently no such argument is made, which makes it look like the commit just blindly enforces "first argument of a method must be named self".

Please clarify in what way this commit improves the code base.

comment:11 follow-up: Changed 22 months ago by chapoton

comment:12 in reply to: ↑ 11 Changed 22 months ago by nbruin

Replying to chapoton:

This is pep8

https://lgtm.com/rules/910082/

As pep8 explains, these are guidelines not rules. It may be that the instances that this commit changes arose from uninformed copying of using "not self" in other places, in which case changing them might make sense.

But I know that at least in the coercion framework, there are places where using "left" and "right" was done very consciously to emphasize that the order of the arguments might NOT have a "self" first and/or because the code expressed intent better with more semantic naming.

In a method that doesn't change attributes on "self", it can be pretty irrelevant if the first argument is the instance the wrapper through which the function is called is bound to.

As PEP8 points out: "A Foolish Consistency is the Hobgoblin of Little Minds". I think there is a real possibility that (some of) the instances changed in the commit here are intentionally deviating from the general quideline. Perhaps putting a comment with them "first argument named "left" for a reason" helps future PEP-8 fervour.

(spacing etc. is less controversial in my mind, but if you're changing names in code, you're actually messing with the expression of semantics the original author put there. You want to make sure that you're informed about such those before making a decision. That probably means it's not worth PEP8-ing this stuff).

EDIT: Looking at some of the examples, I think changing to "self" does make sense in some cases, but then the naming of the other variables would need to be balanced too: "left/right" should be changed to "self/other" to have a balanced naming scheme. It's actually helpful in that code to have a correspondence between the naming of the two parameters. Similarly, the example of "I/J" can be changed to "self/other", but "self/J" is actually less clear in my opinion, because the similarity between the parameters is lost. There may be cases there where "left/right" really is better! I can't tell.

Last edited 22 months ago by nbruin (previous) (diff)

comment:13 Changed 22 months ago by jmantysalo

IMO this should be discussed in the devel list.

(Personally I also found it easier to read for example a+b + c than a + b + c if for some reason it is more logical to think it as (a+b)+c.)

comment:14 Changed 22 months ago by git

  • Commit changed from b21e31c5e77ed96c4854e31306ddec73f3f55955 to 47f6d46f6f59823f1f26df4002eb5b349c08370b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

47f6d46first parameter must be self

comment:15 Changed 22 months ago by chapoton

  • Status changed from needs_info to needs_review

ok, here is a new branch, not touching any method involving the coercion framework, except for some methods about comparison.

This should be less controversial, I hope.

comment:16 follow-up: Changed 22 months ago by chapoton

green bot. Nils, do you approve this version ?

comment:17 in reply to: ↑ 16 Changed 22 months ago by nbruin

Replying to chapoton:

green bot. Nils, do you approve this version ?

No, as I wrote, in several cases (and some remain in the current commit) there are parameters that have highly associated names I/J, left/right etc. That's actually an important part of the legibility of the code. By changing the first parameter to self you break that association, which makes the code less clear, in my opinion. So that's a deterioration in legibility, because a guideline is applied superficially.

I think in those cases, if the author would have chosen self as a first argument, they would have chosen an associated name, like other for the other. Using self/other has the advantage that the first parameter is now called self, which looks familiar. The name other at least suggests it's something that plays a similar role to what self plays, and also occurs a lot (at least in the sage codebase), so looks familiar too. Whether other appropriately describes the role of the parameter (in coercion it could, because it's often just the "other" object involved in a binary operator, so it remains to to see what can be done with it).

Personally I wouldn't touch these files just to force compliance with a guideline, but might bring such guidelines up in review of new code, and might make changes in a file I'm touching anyway.

(I have no beef with the single parameter functions, but since they're usually quite trivial I don't think there's a legibility issue to begin with. The only thing is precedent for future code)

comment:18 Changed 22 months ago by git

  • Commit changed from 47f6d46f6f59823f1f26df4002eb5b349c08370b to 62312d95a7e5639a7c379d2f7d609ef79906fc17

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

62312d9first parameter must be self

comment:19 Changed 22 months ago by git

  • Commit changed from 62312d95a7e5639a7c379d2f7d609ef79906fc17 to 45616f74ff707a968b1d91fbbb92186ba1593e05

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

45616f7first parameter must be self

comment:20 Changed 22 months ago by git

  • Commit changed from 45616f74ff707a968b1d91fbbb92186ba1593e05 to 0620c10164128419db1e1a163a600937dcd6184b

Branch pushed to git repo; I updated commit sha1. New commits:

0620c10first parameter must be self

comment:21 Changed 22 months ago by chapoton

ok. Here is a branch with much less changes, that I hope will be ok for you. I have been careful to change right to other when I was changing left to self. So consistency is preserved. I also checked that the doc was coherently fixed.

Please review

Last edited 22 months ago by chapoton (previous) (diff)

comment:22 Changed 22 months ago by nbruin

  • Status changed from needs_review to positive_review

Ok, bot is green, the ticket accomplishes something in a direction someone is passionate about, and I think there's now a reasonable sign care has been taken to not harm the code base in other ways. So: positive review.

comment:23 Changed 22 months ago by vbraun

  • Branch changed from u/chapoton/29083 to 0620c10164128419db1e1a163a600937dcd6184b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.