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.

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

Flatten options #508

Merged
merged 5 commits into from
Apr 21, 2017
Merged

Flatten options #508

merged 5 commits into from
Apr 21, 2017

Conversation

boopathi
Copy link
Member

@boopathi boopathi commented Apr 19, 2017 •
edited
Loading

  • Removes Option Manager
  • Flattens options - having a tree of options is complex for the user as well as handling it
  • Now every plugin gets its own option name and it can be toggled or passed its own options

@codecov
Copy link

codecov bot commented Apr 19, 2017 •
edited
Loading

Codecov Report

Merging #508 into master will increase coverage by 0.41%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
+ Coverage   82.64%   83.06%   +0.41%     
==========================================
  Files          35       34       -1     
  Lines        2633     2521     -112     
  Branches      924      896      -28     
==========================================
- Hits         2176     2094      -82     
+ Misses        279      255      -24     
+ Partials      178      172       -6
Impacted Files Coverage Δ
packages/babel-preset-babili/src/index.js 92.59% <90.47%> (+15.31%) ⬆️

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 021eead...71cdac0. Read the comment docs.

@boopathi boopathi added Tag: Breaking Change Pull Request breaks the API Tag: Polish Pull Request for formatting, style changes, code cleanups, comments etc... labels Apr 19, 2017
vigneshshanmugam
vigneshshanmugam reviewed Apr 20, 2017
View reviewed changes
packages/babel-preset-babili/src/index.js
proxy("keepFnName", [optionsMap.mangle, optionsMap.deadcode]),
// validate options
const validOptions = [...PLUGINS.map(p => p[0]), ...Object.keys(PROXIES)];
for (let name in opts) {
Copy link
Member

@vigneshshanmugam vigneshshanmugam Apr 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

of instead of in?

Copy link
Member Author

Choose a reason for hiding this comment

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

The accessor is opts[name] and opts is user passed. So better to validate all that we access instead of just accessing the own properties.

Copy link
Member

Choose a reason for hiding this comment

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

Alright.

@vigneshshanmugam
Copy link
Member

vigneshshanmugam commented Apr 20, 2017 •
edited
Loading

Looks much cleaner and easier to validate/handle.

@boopathi boopathi merged commit 369d05e into master Apr 21, 2017
@boopathi boopathi deleted the options-1 branch April 21, 2017 15:10
@renovate renovate bot mentioned this pull request Feb 7, 2020
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vigneshshanmugam vigneshshanmugam vigneshshanmugam approved these changes

Assignees
No one assigned
Labels
Tag: Breaking Change Pull Request breaks the API Tag: Polish Pull Request for formatting, style changes, code cleanups, comments etc...
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@boopathi @vigneshshanmugam