search

Mathics3 / mathics-core

An open-source Mathematica. This repository contains the Python modules for WL Built-in functions, variables, core primitives, e.g. Symbol, a parser to create Expressions, and an evaluator to execute them.
https://mathics.org
Other
786 stars 48 forks source link

Starting to tidy up mathics.core.pattern #1086

Closed mmatera closed 2 months ago

mmatera commented 2 months ago

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.

rocky commented 2 months ago

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.

mmatera commented 2 months ago

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.

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.

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.

OK

rocky commented 2 months ago

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?

rocky commented 2 months ago

Concerning the https://github.com/Mathics3/mathics-core/actions/runs/10905508459/job/30264517603?pr=1086 failure, I suspect that various linters detect this too.

rocky commented 2 months ago

@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.

mmatera commented 2 months ago

@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

mmatera commented 2 months ago

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?

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 commented 2 months ago

@rocky, I think this is ready now. In a next PR I will go over mathics.builtin.patterns.py doing the same.

rocky commented 2 months ago

@rocky, I think this is ready now. In a next PR I will go over mathics.builtin.patterns.py doing the same.

Thanks - give me another day or so to try this out.

rocky commented 2 months ago

LGTM - let's merge and iterate. The developer docs and debugger will now need to track these changes.