Skip to content

Comments

Mathics scanner import#1117

Merged
rocky merged 26 commits intomasterfrom
mathics-scanner-import
Jan 25, 2021
Merged

Mathics scanner import#1117
rocky merged 26 commits intomasterfrom
mathics-scanner-import

Conversation

@rocky
Copy link
Member

@rocky rocky commented Jan 18, 2021

This time for sure!

@rocky rocky requested review from GarkGarcia and mmatera January 18, 2021 22:01
@rocky rocky mentioned this pull request Jan 18, 2021
@rocky rocky force-pushed the mathics-scanner-import branch from a8d4d76 to cac5a37 Compare January 18, 2021 22:29
Copy link
Contributor

@mmatera mmatera left a comment

Choose a reason for hiding this comment

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

I see you have move to MathicsScanner more than the characters conversion table. OK, it is more ambitions that what I was envised, but it seems be useful. The only "issue" that I find on this is that if you want to modify the semantic of a symbol or add new operators, you would have to modify two projects, coordinately. However, I think that is not something that we have to do frequently, so I think this will work.
So, when the parser and mathics pass all the tests, I am OK to merge it.

@rocky rocky force-pushed the mathics-scanner-import branch from cac5a37 to 2a9f312 Compare January 18, 2021 22:37
@mmatera
Copy link
Contributor

mmatera commented Jan 18, 2021

BTW, great work!

@rocky
Copy link
Member Author

rocky commented Jan 18, 2021

I see you have move to MathicsScanner more than the characters conversion table.

As it says in the README.rst for the mathics scanner, it would be good to use this in the pygments tokenizer.

OK, it is more ambitions that what I was envised, but it seems be useful. The only "issue" that I find on this is that if you want to modify the semantic of a symbol or add new operators, you would have to modify two projects, coordinately.

Yes, that is a downside. It also requires a little more sophisication of the developers. But I think/hope we can handle it.

I wanted to move out the parser too, however right now it is a little too tied right now to AST building, and Mathics objects like Number, and so on. So at least to my mind that's for later.

However, I think that is not something that we have to do frequently, so I think this will work.
So, when the parser and mathics pass all the tests, I am OK to merge it.

Great!

@rocky
Copy link
Member Author

rocky commented Jan 18, 2021

BTW, great work!

Actually, @GarkGarcia deserves the lion's share of the credit here.

@GarkGarcia
Copy link
Contributor

The only "issue" that I find on this is that if you want to modify the semantic of a symbol or add new operators, you would have to modify two projects, coordinately. However, I think that is not something that we have to do frequently, so I think this will work.

I don't like this too. We could parameterize the functions of the tokenizer that use the table of operators. That way there's a single source of truth. I'm not sure if it's worth it though.

@rocky This LGTM, but we should import names_wildcard, base_name_pattern and full_names_pattern directly from mathics_scanner.tolenizer since mathics_scanner.letters and mathics_scanner.letterlikes aren't exported anymore since we merged Mathics3/mathics-scanner#3 (I took the time to transfer the code to there). I'll get this fixed and then we can merge this.

GarkGarcia added a commit that referenced this pull request Jan 19, 2021
@rocky
Copy link
Member Author

rocky commented Jan 19, 2021

The only "issue" that I find on this is that if you want to modify the semantic of a symbol or add new operators, you would have to modify two projects, coordinately. However, I think that is not something that we have to do frequently, so I think this will work.

I don't like this too. We could parameterize the functions of the tokenizer that use the table of operators. That way there's a single source of truth. I'm not sure if it's worth it though.

@rocky This LGTM, but we should import names_wildcard, base_name_pattern and full_names_pattern directly from mathics_scanner.tolenizer since mathics_scanner.letters and mathics_scanner.letterlikes aren't exported anymore since we merged Mathics3/mathics-scanner#3 (I took the time to transfer the code to there). I'll get this fixed and then we can merge this.

Ok. Thanks.

I do think more will be added to the YAML file and that it is worth the effort. However at least for me, I need to spread the effort out over a longer period of time since need to limit the amount of time spent per week.

More about this some other time...

@GarkGarcia
Copy link
Contributor

GarkGarcia commented Jan 19, 2021

@rocky The PyYAML thing is really starting to annoy me, I had to basically add a line to each CI configuration file saying "install PyYAML" before we can do anything else.

A simpler way add this to make and use that. I have no doubt that down the line we can find something even better though.

Copy link
Member Author

@rocky rocky left a comment

Choose a reason for hiding this comment

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

Thanks

@rocky rocky marked this pull request as draft January 23, 2021 03:37
@rocky rocky force-pushed the mathics-scanner-import branch 3 times, most recently from 421bd73 to f60539a Compare January 23, 2021 20:11
@rocky rocky force-pushed the mathics-scanner-import branch from f60539a to b041d41 Compare January 23, 2021 20:17
@rocky rocky marked this pull request as ready for review January 23, 2021 21:10
@rocky rocky merged commit 8c4b80c into master Jan 25, 2021
mmatera pushed a commit that referenced this pull request Jan 26, 2021
@rocky rocky deleted the mathics-scanner-import branch February 7, 2021 00:53
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