Skip to content

[add] config #103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Nov 21, 2022
Merged

[add] config #103

merged 10 commits into from
Nov 21, 2022

Conversation

chunibyo-wly
Copy link
Contributor

But I don't know why we need to freeze a config object?
It's looked weired forcing use merge to update configuration?

@chunibyo-wly chunibyo-wly requested a review from odashi as a code owner November 14, 2022 17:00
@odashi
Copy link
Collaborator

odashi commented Nov 14, 2022

Ref: #66

@chunibyo-wly Thanks!

Just a comment about "why we need to freeze": it is basically because immutability keeps ease of maintenance and reproduction.
Always generating a fresh struct keeps the entire library unchaged, i.e., we can guarantee latexify.function has no side effect and we could get exactly the same result every time we called latexify.function even it is invoked from multiple threads. Immutability is also applied in the entire process of this library, especially tree transformation chains in get_latex: all transformers always return a new AST (with references to possibly old subtrees for efficiency).

We may add some functionality to override the "global" config in the future. This kind of functionality should be explicitly mentioned that "invoking this method causes side effect and not thread safe."

@odashi odashi added this to the v0.3 milestone Nov 15, 2022
Copy link
Collaborator

@odashi odashi left a comment

Choose a reason for hiding this comment

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

Almost fine, and there are only trivial comments remaining.

@odashi
Copy link
Collaborator

odashi commented Nov 16, 2022

By the way, there are several pull requests that may affect config so there would be some collisions when merging. I guess it is better to merge this pull request first, and request others to merge main after merging this one.

@odashi
Copy link
Collaborator

odashi commented Nov 21, 2022

@chunibyo-wly Hi, I like this change, and would like to merge it soon! If you could resolve the comments above, I'd really appreciate it.

@chunibyo-wly
Copy link
Contributor Author

sorry, I forgot to check my email

Copy link
Contributor

@ZibingZhang ZibingZhang left a comment

Choose a reason for hiding this comment

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

Since #125 has been merged in, there's a new argument expand_functions in get_latex.

Could you update the Config class to include that as well?

@odashi
Copy link
Collaborator

odashi commented Nov 21, 2022

@ZibingZhang Yeah, but it could be processed after merging this PR. We need to apply the change (because it affects the interface of get_latex.

Copy link
Collaborator

@odashi odashi left a comment

Choose a reason for hiding this comment

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

I'd approve this. maybe we need a follow-up PR to introduce a new flag (or directly applying it here)

@odashi
Copy link
Collaborator

odashi commented Nov 21, 2022

I applied a monkey patch to remain expand_functions as is to merge this PR. After fixing the linter error I will merge it.

@odashi
Copy link
Collaborator

odashi commented Nov 21, 2022

@chunibyo-wly Thanks for your work! I think this is a very important change for this repository.

@odashi odashi merged commit 03ea9e6 into google:main Nov 21, 2022
@odashi odashi added the feature label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants