Starting to tidy up mathics.core.pattern #1086
Conversation
| @@ -1,6 +1,11 @@ | |||
| # cython: language_level=3 | |||
There was a problem hiding this comment.
BTW - I think we should remove cython. At this point it is probably not speeding things up and getting in the way.
There was a problem hiding this comment.
For this, Cython was useful in detecting some errors when I renamed some arguments. Maybe we can remove this later.
mathics/core/pattern.py
Outdated
| count += 1 | ||
| return count | ||
|
|
||
| def sort(self): | ||
| """Sot the elements according to their sort key""" |
|
As I look over I see lots of opportunity for adding type annotations. But one step at a time. See comments and merge when you are satisfied. Also consider making a pass over https://github.com/Mathics3/mathics-development-guide/blob/master/docs/extending/code-overview/pattern-matching.rst (but note that this section is going to be moved up a level out of "Extending" and its own section. |
Fist, I need to be sure about what are the parameters that the functions and methods are receiving. Also, one of the complaints of the linter is that many of these functions have too many parameters. Probably a first step would be to collect all these parameters into a dictionary or a tuple, and then establish the typing.
OK |
mmatera
force-pushed
the
tidyup_core_pattern
branch
from
99c2000
to
3b0e96f
Compare
|
Something to think about: can we write py tests or unit tests for the pattern module that works in isolation; that is, it doesn't get tested via expression evaluation but instead is narrowed to patterns or even in some cases the specific methods of Pattern and its subclasses? |
|
Concerning the https://github.com/Mathics3/mathics-core/actions/runs/10905508459/job/30264517603?pr=1086 failure, I suspect that various linters detect this too. |
|
@mmatera When you think things are stable, let me know. I'd like to checkout this branch and look it over to see if this helps me understand how patterns work better. Thanks. |
I am nearly to finish: I am trying to determine the right annotations using the errors reported by Cython. I'll let you when I finish |
Yes, that would be ideal. The problem is that I still have some problems understanding what some parts of this code do. I hope these changes made it easier to investigate. |
mmatera
force-pushed
the
tidyup_core_pattern
branch
from
3de40c0
to
afa03e4
Compare
|
@rocky, I think this is ready now. In a next PR I will go over |
Thanks - give me another day or so to try this out. |
Here are some minor things noticed in looking over this module. Pattern -> BasePattern; Pattern_ -> Pattern.
|
LGTM - let's merge and iterate. The developer docs and debugger will now need to track these changes. |
In line with the last changes proposed by @rocky, I was doing a pass over mathics.core.pattern and fixing some issues reported by the linter.