Skip to content
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

feat: allow disabling maker in config #2754

Merged
merged 3 commits into from
Jun 9, 2022

Conversation

erickzhao
Copy link
Member

@erickzhao erickzhao commented Mar 3, 2022

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

Closes #1632

@erickzhao erickzhao force-pushed the feat/allow-disable-maker-config branch from ef05dda to c66f82d Compare March 3, 2022 00:15
@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #2754 (2e5ded0) into master (204ec25) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2754      +/-   ##
==========================================
- Coverage   74.06%   73.94%   -0.12%     
==========================================
  Files          77       77              
  Lines        2356     2357       +1     
  Branches      436      437       +1     
==========================================
- Hits         1745     1743       -2     
- Misses        462      464       +2     
- Partials      149      150       +1     
Impacted Files Coverage Δ
packages/api/core/src/api/make.ts 81.81% <100.00%> (+0.18%) ⬆️
packages/maker/appx/src/MakerAppX.ts 64.28% <0.00%> (-5.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 204ec25...2e5ded0. Read the comment docs.

@malept
Copy link
Member

malept commented Mar 3, 2022

🚲 🏚️ : I generally prefer having positive config options (enabled) vs. negative config options (disabled, skip). Was a positive config option name considered?

@erickzhao erickzhao marked this pull request as ready for review March 3, 2022 19:48
@erickzhao
Copy link
Member Author

Was a positive config option name considered?

I just wanted to do the impl from the original issue, but we do have a skipPackage in the make config already and it might be nice to stay consistent: https://js.electronforge.io/api/core/interfaces/makeoptions#skippackage

@malept
Copy link
Member

malept commented Mar 3, 2022

Was a positive config option name considered?

I just wanted to do the impl from the original issue, but we do have a skipPackage in the make config already and it might be nice to stay consistent: https://js.electronforge.io/api/core/interfaces/makeoptions#skippackage

I'd generally argue that those concepts are different enough that consistency in this case is a little weird. skipPackage is a make API/CLI option, whereas skip is a specific maker config option.

Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

LGTM - test failing is a CodeCov difference, which I'm not concerned about

@VerteDinde VerteDinde merged commit 6977740 into master Jun 9, 2022
@VerteDinde VerteDinde deleted the feat/allow-disable-maker-config branch June 9, 2022 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow disabling configured makers via config
4 participants