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

Allow falsey, yet valid options for codeFrameColumns() #7341

Merged
merged 2 commits into from
Feb 8, 2018

Conversation

hulkish
Copy link
Contributor

@hulkish hulkish commented Feb 6, 2018

Allow for overriding default linesAbove/linesBelow values.

Q                       A
Fixed Issues? Fixes #7340
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes + Yes
Documentation PR No
Any Dependency Changes? No
License MIT

Improved option defaulting logic, small change.

const linesAbove = opts.linesAbove || 2;
const linesBelow = opts.linesBelow || 3;
const linesAbove = isNaN(opts.linesAbove) ? 2 : opts.linesAbove;
const linesBelow = isNaN(opts.linesBelow) ? 3 : opts.linesBelow;
Copy link
Member

@Andarist Andarist Feb 6, 2018

Choose a reason for hiding this comment

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

isNaN is imho a legacy API and should not be used nowadays, not sure what the intention was but I guess this should be fine, right?

const linesAbove = typeof opts.linesAbove !== 'number' ? 2 : opts.linesAbove;
const linesBelow = typeof opts.linesBelow !== 'number' ? 3 : opts.linesBelow;

Copy link
Member

Choose a reason for hiding this comment

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

seems like it's just to allow 0

Copy link
Contributor Author

@hulkish hulkish Feb 6, 2018

Choose a reason for hiding this comment

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

yes, to allow 0. I agree about the isNaN usage - i would have preferred to use destructuring with defaults. But, guess I was going for lowest impacting change lol.

I'd be happy to replace isNaN if you guys prefer?

@hzoo hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Feb 6, 2018
Allow for overriding default linesAbove/linesBelow values.
@hulkish
Copy link
Contributor Author

hulkish commented Feb 6, 2018

@Andarist better?

@Andarist
Copy link
Member

Andarist commented Feb 6, 2018

@hulkish Yeah, that works too. opts was not guarded before, so I'm not sure if || {} is needed here though.

hulkish pushed a commit to hulkish/babel that referenced this pull request Feb 6, 2018
@hzoo
Copy link
Member

hzoo commented Feb 8, 2018

Do we just need to rebase this? Since the other prs are passing

@nicolo-ribaudo
Copy link
Member

There is no need to rebase: it will work when merged, since the failing test has been fixed in code not touched by this PR.

@hzoo hzoo merged commit a01007a into babel:master Feb 8, 2018
@hzoo
Copy link
Member

hzoo commented Feb 8, 2018

thanks @hulkish! good find

@rajasekarm rajasekarm deleted the patch-2 branch February 9, 2018 03:21
@rajasekarm rajasekarm restored the patch-2 branch February 9, 2018 03:22
aminmarashi pushed a commit to aminmarashi/babel that referenced this pull request Mar 17, 2018
Allow for overriding default linesAbove/linesBelow values.
@apepper
Copy link
Contributor

apepper commented May 24, 2018

When will there be a release of babel-code-frame containing this bugfix?

@nicolo-ribaudo
Copy link
Member

You can try it in Babel 7 beta (@babel/code-frame)

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for overriding default linesAbove/linesBelow options.
6 participants