-
Notifications
You must be signed in to change notification settings - Fork 398
[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
[add] config #103
Conversation
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. 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." |
Co-authored-by: Yusuke Oda <[email protected]>
There was a problem hiding this 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.
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 |
@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. |
sorry, I forgot to check my email |
Co-authored-by: Yusuke Oda <[email protected]>
There was a problem hiding this 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?
@ZibingZhang |
There was a problem hiding this 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)
I applied a monkey patch to remain |
@chunibyo-wly Thanks for your work! I think this is a very important change for this repository. |
But I don't know why we need to freeze a config object?
It's looked weired forcing use merge to update configuration?