-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
refactor(server): move filename option from createConfig to API #2113
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2113 +/- ##
==========================================
+ Coverage 92.56% 92.58% +0.01%
==========================================
Files 33 33
Lines 1265 1268 +3
Branches 361 363 +2
==========================================
+ Hits 1171 1174 +3
Misses 87 87
Partials 7 7
Continue to review full report at Codecov.
|
/cc @hiroppy what do you think? |
I think we solve this issue just by moving to |
The only reason for the util is to make testing easier. I was following the style that I did here: #2099. However, I'm not convinced these things really belong in A more uniform idea that I have is:I make a helper |
d0c07dd
to
7e37db5
Compare
/cc @evilebottnawi @hiroppy I switched to using Do you think I should proceed by working through these options one PR at a time, or by moving many in 1 PR? |
I'm fine either way. How much is the amount of changes? |
@hiroppy Sorry for late response. I'm essentially just moving everything from |
closing in favor of #2174 |
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
Move default fallback of
options.filename
out ofcreateConfig
, sincecreateConfig
is only used on the CLI.Breaking Changes
If
filename
is not set, it will now default to the compiler'soptions.output.filename
. However, havingfilename
set will have no effect unlesslazy
is enabled, so this should not break anything. Does this warrant moving this tonext
?Additional Info