Mentions légales du service

Skip to content
Snippets Groups Projects

Add the __add__ function as __sub___ symetric

Closed MERCIER Michael requested to merge implement_add_as_sub_symetric into master

To keep the interface consitent: I use sub to remove a procset from an other now I can do the symetric call with add to increase a procset with an other

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Why not, I however have mixed feelings about this.

    The main reason why there is no __add__ as of today is to stick with the interface of standard containers. In the standard library, the __add__ and __or__ operations on iterable respectively are concatenation and union. The concatenation is meaningless with sets, while the union is properly defined.

    Last point, rather than creating a whole new __add__ method, you can just write __add__ = __or__ in the definition of the class. (See the definition of __copy__ for an aliasing example). Also, as this is an alias of __or__, I would declare it right after __or__ rather than next to the definition of __sub__.

    Some minor points:

    • Note that it is already implemented as __or__, you use it in your definition ;)
    • This may clash with the already existing add method: this however is not the best name. Now that you point this change, maybe the existing add would be better named insert. This point is more about a possible rename than an argument against you proposal.
  • It's just an interface issue really: For me it it not intuitive to use or to do a union, If we need to use operators + and - seems more obvious. But I think using the plain text methods (like in interval_set) instead of operators is more explicit. The problem is that having several function to do the same thing goes against the python philosophy...

    Still, if I rewrite my merge request to define plain text methods (union, intersection, difference, ...) as alias for operator will you accept it

    I agree about the add/insert comment, I think insert is more meaningful.

  • About the interface, I agree it is a personal matter ;)

    Still, if I rewrite my merge request to define plain text methods (union, intersection, difference, ...) as alias for operator will you accept it

    Actually, this is planned. Each dunder method for set operation is supposed to have a plain text method that support the operation with many sets (check docs/api.rst). These are not implemented yet, it'd be a useful contribution :smiley:


    I agree about the add/insert comment, I think insert is more meaningful.

    I'll update the API accordingly in the next version.

  • By the way, do not feel obliged to implement all of them. If you implement only the one you do need in this pull request, it is fine for me.

  • mentioned in issue #7 (closed)

  • Closing this MR, as full support for set operations (operators and plain-text methods) is now implemented.

Please register or sign in to reply
Loading