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

change '__len__' method of propositional formula to 'length' #32148

Closed
DaveWitteMorris opened this issue Jul 7, 2021 · 12 comments
Closed

change '__len__' method of propositional formula to 'length' #32148

DaveWitteMorris opened this issue Jul 7, 2021 · 12 comments

Comments

@DaveWitteMorris
Copy link
Member

#28053 added a __len__ method to propositional formulas, so that len(f) would return the length of the formula. However, it was pointed out in #29738 that only containers should have a len. So the method should be renamed to length.

sage: f = propcalc.formula("a -> b")
sage: f.length()
3

For now, __len__ is being retained as an alias.

Component: symbolics

Keywords: boolean formula, len, length

Author: Dave Morris

Branch/Commit: 59cc054

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/32148

@DaveWitteMorris
Copy link
Member Author

Branch: public/32148

@DaveWitteMorris
Copy link
Member Author

Commit: 551f2cc

@DaveWitteMorris
Copy link
Member Author

New commits:

551f2cctrac 32148 length of propositional formula

@tscrim
Copy link
Collaborator

tscrim commented Jul 8, 2021

comment:3

I am fine with everything except deprecating this as a __len__. I think we should take a more incremental approach here and just add the alias, but leave it to #29738 once more concrete decisions have been reached before deprecating this behavior.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2021

Changed commit from 551f2cc to 59cc054

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2021

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

59cc054do not deprecate __len__

@DaveWitteMorris

This comment has been minimized.

@DaveWitteMorris
Copy link
Member Author

comment:5

Thanks for looking at this. I'm sure this feature is rarely used, so postponing the deprecation is fine with me.

@tscrim
Copy link
Collaborator

tscrim commented Jul 9, 2021

comment:6

Thank you.

@tscrim
Copy link
Collaborator

tscrim commented Jul 9, 2021

Reviewer: Travis Scrimshaw

@DaveWitteMorris
Copy link
Member Author

comment:7

Thanks!

@vbraun
Copy link
Member

vbraun commented Jul 24, 2021

Changed branch from public/32148 to 59cc054

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

No branches or pull requests

3 participants