Opened 4 years ago

Closed 4 years ago

#22501 closed defect (fixed)

Make it easier to customize pexpect interfaces

Reported by: SimonKing Owned by:
Priority: major Milestone: sage-7.6
Component: interfaces Keywords: pexpect, days84
Cc: Merged in:
Authors: Simon King Reviewers: Jeroen Demeyer, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 52226e2 (Commits) Commit: 52226e2908447b8d26a2578b1f8fb6751f34c5a5
Dependencies: Stopgaps:

Description (last modified by SimonKing)

While working on #22452, I found a couple of issues that might be dealt with in a separate ticket.

  • The pexpect interfaces and the interface elements inherit from Parent resp. from Element. But they override __repr__, which is bad. So, I change __repr__ into single underscore methods in all interfaces.
  • The default implementation of _true_symbol and some others rely on the assumption the self.eval(...) returns a string representation when applied to an integer or bool. But this assumption is violated for Polymake. However, self.get(...) is supposed to return a string representation for everything. I am stating these specifications in the doc and replace eval with get where needed.
  • For interface elements, I replace self.parent() by self._check_valid() in a couple of places, since that returns the parent after testing that it is still valid.
  • In the string representation of an interface element of a crashed interface, I show the crash message, so that it becomes a bit clearer what has happened.

Change History (94)

comment:1 Changed 4 years ago by SimonKing

There is one more thing. The methods _left_list_delim, _right_func_delim and similar are currently only used to make the string representation of an interface element a bit more similar to Python. However, it is conceivable that they could be used in a nice default implementation of __getitem__ - but I am not sure if such a default implementation would really be of wide use.

comment:2 Changed 4 years ago by SimonKing

  • Keywords days84 added

comment:3 Changed 4 years ago by SimonKing

  • Branch set to u/SimonKing/make_it_easier_to_customize_pexpect_interfaces

comment:4 Changed 4 years ago by SimonKing

  • Commit set to 6580782c527c589bb1c37bc1102d18146a6d380e

The first commit implements and uses _get_primitive. Next, I'll add tests.


New commits:

6580782Add ._get_primitive to pexpect interfaces

comment:5 Changed 4 years ago by git

  • Commit changed from 6580782c527c589bb1c37bc1102d18146a6d380e to 33b53f3e5314c37f5e4ce511dd8aac0c66c78116

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

33b53f3Add documentation to _get_primitive

comment:6 Changed 4 years ago by git

  • Commit changed from 33b53f3e5314c37f5e4ce511dd8aac0c66c78116 to d78b239eb5244f9180e150aff84d254e85d6626c

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

d78b239'Implement string representation by single underscore methods': Apply this mantra to interfaces

comment:7 Changed 4 years ago by SimonKing

I have now also sanitised string representation of pexpect elements. "Sanitise" means: There is a default __repr__ method that is not supposed to be overridden, and if anything needs to be overridden then it is the single underscore _repr_. The change mainly consisted how invalid objects are printed.

Since I am not particularly eager to remove the superfluous functions mentioned in the ticket description, I'll let this be "needs review" --- now I have the prerequisites for #22452.


New commits:

d78b239'Implement string representation by single underscore methods': Apply this mantra to interfaces

comment:8 Changed 4 years ago by SimonKing

  • Status changed from new to needs_review

comment:9 follow-up: Changed 4 years ago by jdemeyer

  • Authors set to Simon King
  • Status changed from needs_review to needs_work

I don't think that you should remove doctests.

If you remove a method which is no longer needed, then move the doctests somewhere else (say, at the top of the module if you don't see a better place) instead of removing them.

comment:10 follow-ups: Changed 4 years ago by jdemeyer

Here

        except ValueError:
            return '(invalid {} object -- defined in terms of closed session)'.format(self.parent())

you should use the exception message instead of hardcoding "defined in terms of closed session". Maybe there are other reasons why a ValueError can be raised. And it might also make sense to replace except ValueError by except Exception.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:11 in reply to: ↑ 9 Changed 4 years ago by SimonKing

Replying to jdemeyer:

I don't think that you should remove doctests.

If you remove a method which is no longer needed, then move the doctests somewhere else (say, at the top of the module if you don't see a better place) instead of removing them.

OK, good point. Actually my plan was to do exactly as you said, but I simply forgot it.

comment:12 Changed 4 years ago by jdemeyer

Concerning invalid None object: in case that parent is None, could you use the type instead? Just because None is not a very interesting value.

comment:13 in reply to: ↑ 10 Changed 4 years ago by SimonKing

Replying to jdemeyer:

Here

        except ValueError:
            return '(invalid {} object -- defined in terms of closed session)'.format(self.parent())

you should use the exception message instead of hardcoding "defined in terms of closed session". Maybe there are other reasons why a ValueError can be raised.

No. Here is the code of _check_valid():

    def _check_valid(self):
        """
        Check that this object is valid, i.e., the session in which this
        object is defined is still running. This is relevant for
        interpreters that can't be interrupted via ctrl-C, hence get
        restarted.
        """
        try:
            P = self.parent()
            if P is None:
                raise ValueError("The %s session in which this object was defined is no longer running."%P.name())
        except AttributeError:
            raise ValueError("The session in which this object was defined is no longer running.")
        return P

So, catching the ValueError is exactly the same as saying "the session is no longer running or some noob overrode Element.parent by something that raises a ValueError and thus the whole interface is broken anyway.

comment:14 follow-up: Changed 4 years ago by jdemeyer

Since this whole ticket is about generalizing why would you assume that _check_valid will never be generalized?

comment:15 Changed 4 years ago by git

  • Commit changed from d78b239eb5244f9180e150aff84d254e85d6626c to 9a0bcca9b4ecd6d4f9d2f010c3a3f939200b9711

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

9a0bccaChange string representation of invalid interface elements whose parent is None

comment:16 follow-up: Changed 4 years ago by jdemeyer

I don't like silly accessor methods like _get_custom_name (but that's just a personal opinion). What's wrong with the more explicit

getattr(foo, "__custom_name", None)

instead of

foo._get_custom_name()

However, if you really insist on using _get_custom_name, you should at least allow for it to be overridden and it should be used everywhere to access __custom_name.

comment:17 follow-up: Changed 4 years ago by jdemeyer

I'm not convinced for the need of the method _get_primitive(). Why cannot you just override eval() to do the right thing instead of inventing an additional layer of indirection?

comment:18 Changed 4 years ago by git

  • Commit changed from 9a0bcca9b4ecd6d4f9d2f010c3a3f939200b9711 to 6910107bb1020b3d9f0080683d19dbaed697f0de

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

6910107Keep the tests that have been removed in the previous commits

comment:19 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by SimonKing

Replying to jdemeyer:

Since this whole ticket is about generalizing why would you assume that _check_valid will never be generalized?

If the semantics of _check_valid changes then of course the person who changes it has to deal with the consequences.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 4 years ago by jdemeyer

Replying to SimonKing:

If the semantics of _check_valid changes then of course the person who changes it has to deal with the consequences.

Or you just make a minor change to the code here such that there won't be any bad consequences.

comment:21 in reply to: ↑ 16 ; follow-up: Changed 4 years ago by SimonKing

Replying to jdemeyer:

I don't like silly accessor methods like _get_custom_name (but that's just a personal opinion). What's wrong with the more explicit

getattr(foo, "__custom_name", None)

instead of

foo._get_custom_name()

Name mangling. It simply didn't work (and was the reason why I added that method). One could of course change __custom_name into a less private attribute. But if we keep it private then there should be an accessor method.

Last edited 4 years ago by SimonKing (previous) (diff)

comment:22 in reply to: ↑ 20 Changed 4 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

If the semantics of _check_valid changes then of course the person who changes it has to deal with the consequences.

Or you just make a minor change to the code here such that there won't be any bad consequences.

I wish SageMath had followed that idea in the past. Otherwise my group cohomology package wouldn't have been broken so often...

comment:23 in reply to: ↑ 21 Changed 4 years ago by jdemeyer

Replying to SimonKing:

Name mangling.

That prevents foo.__custom_name but getattr(foo, "__custom_name") should still work.

comment:24 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by SimonKing

Replying to jdemeyer:

I'm not convinced for the need of the method _get_primitive(). Why cannot you just override eval() to do the right thing instead of inventing an additional layer of indirection?

Currently eval and get have a different semantics, which I tried to explain in the documentation that I added. The reason to introduce _get_primitive is performance: If doing MyInterface.eval('1') returns '' then you need a print statement, such as MyInterface.eval('print(1)'). However, if you are in an interface such as Gap or Singular then the print statement is not needed, and of course it is a bit faster to not use the print statement.

Thus, for *efficiency*, the default is to not use the print statement, but for an interface that needs the print statement you are able to override the default.

comment:25 Changed 4 years ago by git

  • Commit changed from 6910107bb1020b3d9f0080683d19dbaed697f0de to cd97ff3d25ca2f6408d21e79ab9ef9d96373dffb

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

cd97ff3Parse error message of _check_valid() in order to cope with unpredictable future semantical changes

comment:26 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by SimonKing

Replying to jdemeyer:

Here

        except ValueError:
            return '(invalid {} object -- defined in terms of closed session)'.format(self.parent())

you should use the exception message instead of hardcoding "defined in terms of closed session".

By the way, how do I use the exception message? If I do

    except ValueError, msg:

then "no longer" in msg doesn't work (ValueError has no __contains__ method) and "no longer" in msg.message gives a DeprecationWarning.

So, am I really supposed to do "no longer" in repr(msg)? Or did you mean I should let the __repr__ method return '(invalid {} object - {})'.format(self.parent() or type(self), msg)?

Maybe there are other reasons why a ValueError can be raised. And it might also make sense to replace except ValueError by except Exception.

I was taught by several people to not catch a general Exception (in contrast to a more specific type of error) unless it is absolutely clear that I do not want that exception to be propagated. And that's not the case here. If a different error occurs then I don't want to hide it.

comment:27 Changed 4 years ago by git

  • Commit changed from cd97ff3d25ca2f6408d21e79ab9ef9d96373dffb to fdaeb2e08e4de89ca5d9f99e7c9486f78693c5e7

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

fdaeb2eInclude error message of validity check into string representation of invalid interface elements

comment:28 Changed 4 years ago by SimonKing

Meanwhile I think it is in fact a good idea to explicitly include the error message of the ValueError raised by self._check_valid() into the string representation of an invalid interface element self. But it really should be ValueError. There is no point in trying to guess future semantical changes to _check_valid. If someone lets it raise a different error than it is his/her job to take care of backward compatibility.


New commits:

fdaeb2eInclude error message of validity check into string representation of invalid interface elements

comment:29 Changed 4 years ago by SimonKing

  • Status changed from needs_work to needs_review

I think I have answered the points that you raised. So, I'll change it back to "needs review".

comment:30 follow-up: Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

What about 16

comment:31 in reply to: ↑ 24 ; follow-up: Changed 4 years ago by jdemeyer

Replying to SimonKing:

Currently eval and get have a different semantics, which I tried to explain in the documentation that I added. The reason to introduce _get_primitive is performance: If doing MyInterface.eval('1') returns '' then you need a print statement, such as MyInterface.eval('print(1)'). However, if you are in an interface such as Gap or Singular then the print statement is not needed, and of course it is a bit faster to not use the print statement.

I guess we are misunderstanding eachother. My point was: why not change SimonKingsInterface.eval() to add the print statement? Why does it need an additional method?

comment:32 in reply to: ↑ 26 Changed 4 years ago by jdemeyer

Replying to SimonKing:

I was taught by several people to not catch a general Exception (in contrast to a more specific type of error) unless it is absolutely clear that I do not want that exception to be propagated.

In general, that's true. I'm just saying that it's debatable whether you do want the exception to be propagated. Anyway, changing this to except Exception was just a suggestion so it's perfectly OK to keep it as except ValueError.

comment:33 in reply to: ↑ 30 Changed 4 years ago by SimonKing

Replying to jdemeyer:

What about 16

I already answered that. I was bitten by name mangling. While

sage: r = singular.ring(0,'(x,y)','dp')
sage: M = singular.matrix(2,2,"(25/47*x^2*y^4 + 63/127*x + 27)^3,y,0,1")
sage: M.rename('T')
sage: M.__custom_name
'T'

does work, hasattr(self,'__custom_name') returned False when I had it in the _repr_ method.

But if you use self.__custom_name in the same module in which the private attribute is defined (namely sage.structure.sage_object) then there is no point to use the accessor method, as it is slower.

Just to be on the safe side (maybe I had a typo such as hasattr(self,'__custon_name')?), I'll try to change it into using self.__custom_name? and tell you what happens.

comment:34 in reply to: ↑ 31 ; follow-up: Changed 4 years ago by SimonKing

Replying to jdemeyer:

I guess we are misunderstanding eachother. My point was: why not change SimonKingsInterface.eval() to add the print statement? Why does it need an additional method?

If you need the print statement then use the (already existing) method .get(), not .eval(). The same applies to Singular:

sage: r = singular.ring(0,'(x,y)','dp')
sage: M = singular.matrix(2,2,"(25/47*x^2*y^4 + 63/127*x + 27)^3,y,0,1")
sage: singular.eval(M._name)
'sage8[1,1]=15625/103823*x^6*y^12+118125/280543*x^5*y^8+50625/2209*x^4*y^8+297675/758063*x^4*y^4+255150/5969*x^3*y^4+54675/47*x^2*y^4+250047/2048383*x^3+321489/16129*x^2+137781/127*x+19683\nsage8[1,2]=y\nsage8[2,1]=0\nsage8[2,2]=1'
sage: singular.get(M._name)
'sage8[1,1],y,\n0,         1 '

comment:35 follow-up: Changed 4 years ago by jdemeyer

Maybe you missed 23. You can override the mangling with getattr()/setattr()/hasattr().

comment:36 in reply to: ↑ 34 ; follow-up: Changed 4 years ago by jdemeyer

Replying to SimonKing:

If you need the print statement then use the (already existing) method .get(), not .eval().

I am getting more and more lost here...

Given what you just said, what's the difference between get() and _get_primitive()?

comment:37 in reply to: ↑ 35 Changed 4 years ago by SimonKing

Replying to jdemeyer:

Maybe you missed 23. You can override the mangling with getattr()/setattr()/hasattr().

Yes, I missed it. Thanks for pointing it out. Next commit will be removal of _get_custom_name.

comment:38 in reply to: ↑ 36 ; follow-up: Changed 4 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

If you need the print statement then use the (already existing) method .get(), not .eval().

I am getting more and more lost here...

Given what you just said, what's the difference between get() and _get_primitive()?

For Singular and Gap, _get_primitive() does the same as eval(), which is fine because primitives (such as integers) in Gap or Singular do not need to be printed in order to get their string representations. So, blindly using get() has a small performance effect.

However, note that this ticket is a dependency for #22452. And in Polymake, you need an explicit print statement even for primitives; thus, blindly using eval() wouldn't do the right thing.

Hence, it is needed to introduce a hook _get_primitive to customise it.

comment:39 in reply to: ↑ 38 ; follow-up: Changed 4 years ago by jdemeyer

Given the explanations that you just made that _get_primitive() is just a more efficient version of get(), I think that _get_primitive() should default to calling get() instead of eval().

comment:40 in reply to: ↑ 39 ; follow-up: Changed 4 years ago by SimonKing

Replying to jdemeyer:

Given the explanations that you just made that _get_primitive() is just a more efficient version of get(), I think that _get_primitive() should default to calling get() instead of eval().

The default, I think, should cover the most frequent case. So far, Polymake is the only interface I am aware of that would need a print statement to show an integer. That's why I chose it to default to eval(), which works for...

sage: gp.eval('1')
'1'
sage: singular.eval('1')
'1'
sage: gap.eval('1')
'1'
sage: maxima.eval('1')
'1'

whereas you will have (with #22452)

sage: polymake.eval('1')
''

comment:41 Changed 4 years ago by git

  • Commit changed from fdaeb2e08e4de89ca5d9f99e7c9486f78693c5e7 to cdb085bf1b55254cb078bec28f8cd7ed19bf41dd

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

cdb085bRevert introduction of _get_custom_name

comment:42 follow-up: Changed 4 years ago by SimonKing

After reverting _get_custom_name in the latest commit, I think the only open question at the moment is what should be the default for _get_primitive, right?


New commits:

cdb085bRevert introduction of _get_custom_name

comment:43 in reply to: ↑ 40 ; follow-up: Changed 4 years ago by jdemeyer

Replying to SimonKing:

Replying to jdemeyer:

Given the explanations that you just made that _get_primitive() is just a more efficient version of get(), I think that _get_primitive() should default to calling get() instead of eval().

The default, I think, should cover the most frequent case.

No, I think the default should cover the case which makes it least likely to make mistakes.

I also think that many interfaces have custom get() methods but don't really need it. For example, GP has a custom get() method calling print() but I don't see how GP would treat variables differently from "primitive" objects.

Also, it is mentally a lot easier to understand if _get_primitive() would default to get(). Until all your extra explanations I found it hard to understand what _get_primitive() is about. If you say that _get_primitive() is just an optimization for get(), then it makes a lot of sense that _get_primitive() would default to get().

comment:44 in reply to: ↑ 42 ; follow-up: Changed 4 years ago by jdemeyer

Replying to SimonKing:

After reverting _get_custom_name in the latest commit, I think the only open question at the moment is what should be the default for _get_primitive, right?

I guess so.

Don't forget the doctests. Speaking of which, you should add a direct doctest for _get_primitive() too.

comment:45 in reply to: ↑ 44 Changed 4 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

After reverting _get_custom_name in the latest commit, I think the only open question at the moment is what should be the default for _get_primitive, right?

I guess so.

Don't forget the doctests. Speaking of which, you should add a direct doctest for _get_primitive() too.

O, right! I forgot to run sage --coverage yet.

comment:46 Changed 4 years ago by git

  • Commit changed from cdb085bf1b55254cb078bec28f8cd7ed19bf41dd to b5df1359fb0ddaf26c35db9820d3bd7158d5046d

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

b5df135Actually remove _get_custom_name, not only its applications

comment:47 in reply to: ↑ 43 ; follow-ups: Changed 4 years ago by SimonKing

Replying to jdemeyer:

I also think that many interfaces have custom get() methods but don't really need it. For example, GP has a custom get() method calling print() but I don't see how GP would treat variables differently from "primitive" objects.

It does treat them differently:

sage: M = random_matrix(ZZ,3,3)
sage: m = gp(M); n = gp(15)
sage: gp.get(m._name), gp.get(n._name)
('[0, -1, 2; 2, -12, 0; -2, 1, 2]', '15')
sage: gp.eval(m._name), gp.eval(n._name)
('\n[ 0  -1 2]\n\n[ 2 -12 0]\n\n[-2   1 2]\n', '15')

Apparently the creator of the interface thought that the latter result is nicer than the former.

Also, it is mentally a lot easier to understand if _get_primitive() would default to get(). Until all your extra explanations I found it hard to understand what _get_primitive() is about. If you say that _get_primitive() is just an optimization for get(), then it makes a lot of sense that _get_primitive() would default to get().

First proposition: _get_primitive_ should be used in the default comparison method.

Rationale: Using eval() in the default comparison means I'll have to override the comparison method in the Polymake interface (doable of course, but I'd like to avoid it). Using get() means to lose performance of comparison (thus, an important operation) in all other interfaces.

Second proposition: If _get_primitive_ is used in comparison methods, it should default to eval.

Rationale: If instead it would default to get, I'd need to override the default in ALL existing interfaces, or I would have to live with a performance loss in comparison.

Counterargument: I do override _get_primitive in Polymake mainly in order to avoid overriding __cmp__ (I also use it in helper methods, but they are cached and thus using get() is fine there). Hence, why do I introduce _get_primitive and override it in Polymake, rather than I change the helper methods to use get() and then override __cmp__ in Polymake?


New commits:

b5df135Actually remove _get_custom_name, not only its applications

comment:48 in reply to: ↑ 47 ; follow-ups: Changed 4 years ago by jdemeyer

Replying to SimonKing:

It does treat them differently:

Right, thanks for actually checking my statements :-)

sage: M = random_matrix(ZZ,3,3)
sage: m = gp(M); n = gp(15)
sage: gp.get(m._name), gp.get(n._name)
('[0, -1, 2; 2, -12, 0; -2, 1, 2]', '15')
sage: gp.eval(m._name), gp.eval(n._name)
('\n[ 0  -1 2]\n\n[ 2 -12 0]\n\n[-2   1 2]\n', '15')

Apparently the creator of the interface thought that the latter result is nicer than the former.

You mean that the former

[0, -1, 2; 2, -12, 0; -2, 1, 2]

is nicer than the latter

[ 0  -1 2]

[ 2 -12 0]

[-2   1 2]

I think it depends a lot on the application. If the output of get() is supposed to be machine-readable, then certainly the former is nicer.

comment:49 in reply to: ↑ 48 Changed 4 years ago by SimonKing

Replying to jdemeyer:

You mean that the former ... is nicer than the latter

Yes, copy-and-paste error (first, I had the code ordered differently).

comment:50 in reply to: ↑ 48 Changed 4 years ago by SimonKing

Replying to jdemeyer:

I think it depends a lot on the application. If the output of get() is supposed to be machine-readable, then certainly the former is nicer.

This I cannot tell. I added some documentation based on the observation the get in all cases that I looked at involves printing, whereas eval in all instances that I looked at does not. So, if the purpose of get() is to get something machine-readable then it hasn't been documented.

comment:51 in reply to: ↑ 47 Changed 4 years ago by SimonKing

Replying to SimonKing:

Counterargument: I do override _get_primitive in Polymake mainly in order to avoid overriding __cmp__ (I also use it in helper methods, but they are cached and thus using get() is fine there). Hence, why do I introduce _get_primitive and override it in Polymake, rather than I change the helper methods to use get() and then override __cmp__ in Polymake?

I found my own counter-argument convincing. Thus, I'll remove _get_primitive in my next commit. But that has to wait -- I have to prepare the talk that I'm about to give at sage days 84...

comment:52 in reply to: ↑ 47 ; follow-up: Changed 4 years ago by jdemeyer

Replying to SimonKing:

Using get() means to lose performance of comparison (thus, an important operation) in all other interfaces.

Are you sure that there is a real performance hit? Or is this just some hypothetical consideration?

comment:53 in reply to: ↑ 52 Changed 4 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

Using get() means to lose performance of comparison (thus, an important operation) in all other interfaces.

Are you sure that there is a real performance hit? Or is this just some hypothetical consideration?

It is hypothetical. And as I said in my previous post I now plan to leave the default implementation of __cmp__ as it is, do not introduce _get_primitive, and override __cmp__ for the upcoming Polymake interface.

comment:54 Changed 4 years ago by SimonKing

  • Description modified (diff)

comment:55 Changed 4 years ago by git

  • Commit changed from b5df1359fb0ddaf26c35db9820d3bd7158d5046d to 6b532293a5807e7ea82f62384cb47f89ea34f7e6

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

6b53229Reverse addition of _get_primitive. Fix __cmp__, bool etc. and their documentation

comment:56 follow-up: Changed 4 years ago by SimonKing

  • Status changed from needs_work to needs_review

I think with the current commit I have addressed the objections that we discussed above. Note that I did not remove the seemingly unused methods mentioned in the ticket description. I hope that's fine.


New commits:

6b53229Reverse addition of _get_primitive. Fix __cmp__, bool etc. and their documentation

comment:57 follow-up: Changed 4 years ago by vdelecroix

What is the point of replacing self.__custom_name by getattr(self, '__custom_name')?

comment:58 in reply to: ↑ 57 ; follow-up: Changed 4 years ago by SimonKing

Replying to vdelecroix:

What is the point of replacing self.__custom_name by getattr(self, '__custom_name')?

I tried self.__custom_name before changing to "getattr", and it didn't work.

comment:59 in reply to: ↑ 58 Changed 4 years ago by jdemeyer

Replying to SimonKing:

Replying to vdelecroix:

What is the point of replacing self.__custom_name by getattr(self, '__custom_name')?

I tried self.__custom_name before changing to "getattr", and it didn't work.

Indeed. __custom_name wouldn't work to the Python's name mangling. So the old code could never have worked...

comment:60 in reply to: ↑ 56 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_info

Replying to SimonKing:

I think with the current commit I have addressed the objections that we discussed above. Note that I did not remove the seemingly unused methods mentioned in the ticket description. I hope that's fine.

I think you should update the ticket description. It's much easier to review a ticket like this when the purpose of the ticket is clear.

comment:61 follow-up: Changed 4 years ago by jdemeyer

Replace

except ValueError, msg

by

except ValueError as msg

comment:62 Changed 4 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_info to needs_review

comment:63 Changed 4 years ago by git

  • Commit changed from 6b532293a5807e7ea82f62384cb47f89ea34f7e6 to 87ad58632e42a3d0c75f7871391012fd8b328f35

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

87ad586Change syntax for catching 'ValueError as...'

comment:64 in reply to: ↑ 61 Changed 4 years ago by SimonKing

Replying to jdemeyer:

Replace

except ValueError, msg

by

except ValueError as msg

I did this in the current commit, and I changed the ticket description to say that I believe the mentioned functions "could be removed, but not on this ticket".

So, I guess that means "needs review" again.

comment:65 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_info

The ticket description isn't up-to-date. For example, it still talks about _get_primitive.

comment:66 Changed 4 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_info to needs_review

comment:67 Changed 4 years ago by SimonKing

  • Description modified (diff)

comment:68 Changed 4 years ago by SimonKing

  • Description modified (diff)

I think the ticket description now clearly states what this ticket is doing.

comment:69 follow-up: Changed 4 years ago by mantepse

Is the fricas interface not touched because it is "experimental"? I just checked that there is a __repr__ method in the FriCAS class, with two underscores. Should that be changed?

comment:70 in reply to: ↑ 69 Changed 4 years ago by SimonKing

Replying to mantepse:

Is the fricas interface not touched because it is "experimental"?

No, I simply missed that one.

I just checked that there is a __repr__ method in the FriCAS class, with two underscores. Should that be changed?

Yes.

comment:71 Changed 4 years ago by SimonKing

Oops! I just realise that there are many double underscore __repr__ that I forgot to change.

Anyway, the interfaces themselves inherit from parent, the objects represented in the interface inherit from element. And that means it should be single underscore, as a rule of thumb.

comment:72 Changed 4 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to remove *all* __repr__

comment:73 follow-up: Changed 4 years ago by jdemeyer

Since you're changing the branch anyway, could you please either remove the whitespace changes or move them to a separate commit? I found it quite annoying when reviewing to see that many whitespace changes.

comment:74 in reply to: ↑ 73 ; follow-up: Changed 4 years ago by SimonKing

Replying to jdemeyer:

Since you're changing the branch anyway, could you please either remove the whitespace changes or move them to a separate commit? I found it quite annoying when reviewing to see that many whitespace changes.

I didn't intend to do whitespace changes, however my editor (geany) is configured to change tabs into four blanks and to remove trailing whitespace when saving a file.

How should I proceed? Shall I rewrite history, force-push and then rebase #22452 on top of it? Or shall I add a commit that re-introduces the whitespace (probably not)?

comment:75 in reply to: ↑ 74 ; follow-ups: Changed 4 years ago by vdelecroix

Replying to SimonKing:

Replying to jdemeyer:

Since you're changing the branch anyway, could you please either remove the whitespace changes or move them to a separate commit? I found it quite annoying when reviewing to see that many whitespace changes.

I didn't intend to do whitespace changes, however my editor (geany) is configured to change tabs into four blanks and to remove trailing whitespace when saving a file.

So the editor is the culprit?

How should I proceed? Shall I rewrite history, force-push and then rebase #22452 on top of it? Or shall I add a commit that re-introduces the whitespace (probably not)?

Force push would even be more trouble now. But I agree that for diff readers, seeing all these tiny modifications is annoying. For the future I would suggest: just start with a first commit that deal with whitespaces and nothing else. Then add non trivial commits on top of that.

comment:76 in reply to: ↑ 75 Changed 4 years ago by SimonKing

Replying to vdelecroix:

I didn't intend to do whitespace changes, however my editor (geany) is configured to change tabs into four blanks and to remove trailing whitespace when saving a file.

So the editor is the culprit?

I said "I didn't intend to...". So, the fact that I have this option by default is a good when creating a new file, but bad when editing an existing file that doesn't conform to the standards that sage has regarding trailing whitespace.

Force push would even be more trouble now. But I agree that for diff readers, seeing all these tiny modifications is annoying. For the future I would suggest: just start with a first commit that deal with whitespaces and nothing else. Then add non trivial commits on top of that.

I am not talking about future. What to do now?

comment:77 in reply to: ↑ 75 ; follow-up: Changed 4 years ago by jdemeyer

Replying to vdelecroix:

Force push would even be more trouble now.

Why?

You should not force-push here and then base #22452. Instead, you should force-push #22452, checkout the commit corresponding to this ticket and force-push that.

That's very easy to do.

comment:78 in reply to: ↑ 77 ; follow-up: Changed 4 years ago by SimonKing

Replying to jdemeyer:

Replying to vdelecroix:

Force push would even be more trouble now.

Why?

You should not force-push here and then base #22452. Instead, you should force-push #22452, checkout the commit corresponding to this ticket and force-push that.

That's very easy to do.

I don't understand that at all. This ticket is a dependency for #22452. Do you suggest to revert the dependencies?

comment:79 in reply to: ↑ 78 ; follow-up: Changed 4 years ago by jdemeyer

Replying to SimonKing:

This ticket is a dependency for #22452. Do you suggest to revert the dependencies?

No. If I understand things correctly, #22452 contains the commit from this ticket (#22501). So you could rebase #22452 but keeping a separation between the commits which really belong to #22501 and the ones which really belong to #22452 (so not squashing everything together).

Then you can force-push #22452. At this point, the branch from #22501 will no longer be a sub-branch of #22452. But that can be fixed by resetting the branch on #22501 to a commit which was pushed on #22452.

I can easily do this myself if you want.

comment:80 in reply to: ↑ 79 Changed 4 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

This ticket is a dependency for #22452. Do you suggest to revert the dependencies?

No. If I understand things correctly, #22452 contains the commit from this ticket (#22501).

Yes, it does.

So you could rebase #22452 but keeping a separation between the commits which really belong to #22501 and the ones which really belong to #22452 (so not squashing everything together).

That's why I suggested to force a change here and then rebase the commits that really belong to #22452 on top of the new forced commit from here (#22501).

Then you can force-push #22452. At this point, the branch from #22501 will no longer be a sub-branch of #22452. But that can be fixed by resetting the branch on #22501 to a commit which was pushed on #22452.

It seems to me that the result is equivalent to what I suggest.

So, my plan now is:

  • Create a new single commit for this ticket (#22501) that removes trailing whitespace only in the lines that have also been edited for other reasons.
  • Force-push the new commit here.
  • Rebase the commits from #22452 that really belong to #22452 on top of the new commit. Push (of course using force) the result there.

comment:81 Changed 4 years ago by git

  • Commit changed from 87ad58632e42a3d0c75f7871391012fd8b328f35 to 5082c96e79a15b34779a0c9b0b43d652cdade31c

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

5082c96Use single underscore _repr_ in interfaces, add more documentation concerning eval and get

comment:82 Changed 4 years ago by SimonKing

  • Status changed from needs_work to needs_review

I guess both tickets are changed so that reviewing can be resumed (just make sure that you check out the new force-changed branches).

comment:83 follow-up: Changed 4 years ago by vdelecroix

Why self._obj._check_valid() is done in _repr_? Could it be moved to __repr__? That way we would have the clearer M = self.parent() in the multiple _repr_ methods.

comment:84 in reply to: ↑ 83 Changed 4 years ago by SimonKing

Replying to vdelecroix:

Why self._obj._check_valid() is done in _repr_? Could it be moved to __repr__? That way we would have the clearer M = self.parent() in the multiple _repr_ methods.

In fact, _check_valid is done in the default (double underscore) __repr__ - see sage.interfaces.interface.InterfaceElement. So, you are right, it should be safe to just do "self.parent()". Will change it in a minute.

comment:85 Changed 4 years ago by git

  • Commit changed from 5082c96e79a15b34779a0c9b0b43d652cdade31c to 52226e2908447b8d26a2578b1f8fb6751f34c5a5

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

52226e2Do not _check_valid in _repr_, since it is done in __repr__

comment:86 Changed 4 years ago by SimonKing

Done.


New commits:

52226e2Do not _check_valid in _repr_, since it is done in __repr__

New commits:

52226e2Do not _check_valid in _repr_, since it is done in __repr__

comment:87 Changed 4 years ago by SimonKing

PS: I will not put this additional commit into #22452, as it isn't relevant there, and I was told to avoid useless merges.

comment:88 Changed 4 years ago by jdemeyer

  • Work issues remove *all* __repr__ deleted

comment:89 Changed 4 years ago by SimonKing

Oddly, the patchbot reports failures. But as much as I can see they are unrelated with this ticket.

comment:90 Changed 4 years ago by vdelecroix

  • Reviewers set to Jeroen Demeyer, Vincent Delecroix

Indeed. It is weird.

I am good with this ticket, if nobody have further comment I will set it to positive review.

comment:91 Changed 4 years ago by mantepse

I am currently recompiling, so I can run the tests of the fricas interface (which still has "experimental" status, unfortunately).

comment:92 Changed 4 years ago by mantepse

OK, fricas tests are passing.

comment:93 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to positive_review

Let it be!

comment:94 Changed 4 years ago by vbraun

  • Branch changed from u/SimonKing/make_it_easier_to_customize_pexpect_interfaces to 52226e2908447b8d26a2578b1f8fb6751f34c5a5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.