-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Remove remaining global config #3099
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ubig/remove_remaining_global_config
xingyaoww
approved these changes
Jul 26, 2024
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.
LGTM! Just a couple minor comments - i think it should be ready to merge once we fixed all the test cases
neubig
changed the title
[WIP] Remove remaining global config
Remove remaining global config
Jul 26, 2024
…ubig/remove_remaining_global_config
I have confirmed that tests pass and the app runs on my local machine, I think this is ready to merge! |
tobitege
reviewed
Jul 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the problem that this fixes or functionality that this introduces? Does it fix any open issues?
This removes the remaining global configuration. Finally done refactoring!
[WIP] because this still needs to be tested, please do not spend lots of time reviewing yet.
Give a summary of what the PR does, explaining any non-trivial design decisions
There are a few non-trivial things:
workspace_base
variable, I removed this because it seems like a bit of an anti-pattern, but we probably need to make sure this doesn't break things.AppConfig
toRuntime
. I want to avoid usingAppConfig
as much as possible, as it requires/provides too much information, and obscures which information is actually used by the class. However, runtimes use lots of information and different runtimes use different types of information at the moment, so this seemed necessary to avoid further extensive refactoring.Other references
Fixes #2758