-
Notifications
You must be signed in to change notification settings - Fork 177
[deprecation clean up] remove firrtl.ExecutionOptionsManager #2422
Conversation
0ea1df3
to
cd44077
Compare
cd44077
to
b44b4ca
Compare
That API had been deprecated and is now removed.
Logger.getGlobalLevel then returns LogLevel.Warn when the current value is LogLevel.None. This preserves the behavior of the default being "Warn" but now uses LogLevel.None to indicate "I'm not setting the value." This resolves issues where it was not possible to tell if annotations were actually setting the log level or if the default level of warn was just being filled in.
783a884
to
145ff9e
Compare
I added a change that effectively reimplements #1305 in a different way. Removing the old version of One might point out that @seldridge I'd appreciate some thoughts from you on this. |
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 think your interpretation of LogLevel.None
is fine.
A future improvement would fully remove LogLevel.None
and set "none" based on "no log level annotation present". Currently, we may be double-encoding this with "no annotation" and LogLevel.None
.
4163048 should depend on #2428
Contributor Checklist
Type of Improvement
API Impact
This removes many APIs that have been deprecated for a long time:
ExecutionOptionsManager and related APIs: ComposableOptions, HasParser, CommonOptions, HasCommonOptions, FirrtlExecutionOptions, HasFirrtlOptions, FirrtlExecutionResult, FirrtlExecutionSuccess, and FirrtlExecutionFailure
It also removes other deprecated APIs using ExecutionOptionsManager: logger.Logger.makeScope and firrtl.stage.DriverCompatibility.firrtlResultView.
Lastly, it deletes non-deprecated OutputConfig, SingleFile, and OneFilePerModule, but these were types only used by ExecutionOptionsManager APIs.
Backend Code Generation Impact
No impact
Desired Merge Strategy
Release Notes
Remove
ExecutionOptionsManager
and related APIs. See #2422 for full list of removed APIsReviewer Checklist (only modified by reviewer)
Please Merge
?