Skip to content

Mostly better code documentation...#164

Merged
rocky merged 4 commits intomasterfrom
documentation-tweaks
Feb 21, 2022
Merged

Mostly better code documentation...#164
rocky merged 4 commits intomasterfrom
documentation-tweaks

Conversation

@rocky
Copy link
Copy Markdown
Member

@rocky rocky commented Feb 21, 2022

some more lea{f,ves} -> element{,s} converted in places we are currently
trying to describe and don't want to have to explain that by X we really
mean Y.

some more lea{f,ves} -> element{,s} converted in places we are currently
trying to describe and don't want to have to explain that by X we really
mean Y.
Copy link
Copy Markdown
Contributor

@TiagoCavalcante TiagoCavalcante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread mathics/core/parser/ast.py Outdated
return '"' + self.value + '"'


# rocky: I really don't get why Filename has to be special, but so be it.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't get it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just because we choose different print representations: Strings are rounded by quotes, and filenames don't.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I guess that makes sense, but then why isn't Filename distinct from Symbol?

Is this how Mathematica does things too? I would have hoped they would say use another pair of delimiters like <Filename>.

I'll add a comment about this upon learning that __repr__ really is the "right" thing to do.

@mmatera
Copy link
Copy Markdown
Contributor

mmatera commented Feb 21, 2022

LGTM

@mmatera
Copy link
Copy Markdown
Contributor

mmatera commented Feb 21, 2022

Side comment: some time ago, I noticed that in the AST module we have classes with names that appears also as BaseExpression s. For example, we have two classes called Symbol. Do we really need these two classes? Could be just one? Otherwise, shouldn't we rename the AST Symbol to avoid confussions?

@rocky
Copy link
Copy Markdown
Member Author

rocky commented Feb 21, 2022

Side comment: some time ago, I noticed that in the AST module we have classes with names that appears also as BaseExpression s. For example, we have two classes called Symbol. Do we really need these two classes? Could be just one? Otherwise, shouldn't we rename the AST Symbol to avoid confusions?

Yeah, I wondered about this too. This may be yet more sloppiness, made harder to find due to a lack of positive description of intent. Or maybe it was the intent. Like Fermat's last theorem, keeping quiet is sometimes be assumed to be brilliance.

Python's duck typing doesn't see this as an error. That the API interfaces are compatible is then probably not a coincidence. (Because if this was not the case, then Python would throw an exception.)

At the Python level, one class is parser.Atom and the other class s expression.Atom. And I could see having these two be the same because they are equivalent being intentional or being viewed as the right thing.

Renaming is a possibility and so is subclassing.

But right now though I'd rather keep this as is and come back to this after we think about it some more. I'll add a comment to note the issue and think about and possibly take action.

that we have two Atom classes. Make note of this, but
defer a decision of what to do for later.
@rocky rocky merged commit 05965aa into master Feb 21, 2022
@rocky rocky deleted the documentation-tweaks branch February 21, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants