Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

feat: allow disabling maker in config #2754

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 9, 2022
Merged

feat: allow disabling maker in config #2754

merged 3 commits into from
Jun 9, 2022

Conversation

erickzhao
Copy link
Member

@erickzhao erickzhao commented Mar 3, 2022 •
edited
Loading

  • 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 •
edited
Loading

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.

VerteDinde
VerteDinde approved these changes Jun 9, 2022
View reviewed changes
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

Reviewers

@VerteDinde VerteDinde VerteDinde approved these changes

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

@malept malept Awaiting requested review from malept

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow disabling configured makers via config

4 participants

@erickzhao @malept @VerteDinde @MarshallOfSound
Morty Proxy This is a proxified and sanitized view of the page, visit original site.