Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

one-line: Allow to specify "next-line" #2410

Closed
wants to merge 4 commits into from

Conversation

andy-hanson
Copy link
Contributor

@andy-hanson andy-hanson commented Mar 26, 2017

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

Before, you would specify:

"one-line": [true, "check-catch", "check-else", "check-finally", "check-open-brace", "check-whitespace"]
"one-line": [true, {
	"catch": "next-line",
	"else": "next-line",
	"finally": "next-line",
	"open-brace": "same-line",
	"whitespace": true
}]

And get code like:

if (so) { // '{' on same line
	
}
// 'else' on next line
else {
	
}

Is there anything you'd like reviewers to focus on?

This is a breaking change to "one-line" as the options have changed.
We could instead create a new rule and deprecate "one-line". (The old name is less accurate now.) Or, we could deprecate the old options style but still handle it in parseOptions.

CHANGELOG.md entry:

TBD

@andy-hanson
Copy link
Contributor Author

Added parsing for old-style options. Shouldn't be a breaking change any more.

@suchanlee
Copy link
Contributor

@andy-hanson are you still working on this PR and relevant?

@andy-hanson
Copy link
Contributor Author

Updated, seems to still work with no change. Note that there is an old-style-options test to test for backwards compatibility.

Copy link

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

@andy-hanson is there any way we can make this less breaking?

please update this branch so we can review it, or close it if no longer relevant. we will close this if we do not hear from you in two weeks.

const OPTION_BRACE = "open-brace";
const OPTION_CATCH = "catch";
const OPTION_ELSE = "else";
const OPTION_FINALLY = "finally";

Choose a reason for hiding this comment

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

changing the option names is super breaking. do we have to do this?

const OPTION_WHITESPACE = "check-whitespace";

type BraceSetting = "same-line" | "next-line";
const OPTION_SAME_LINE: BraceSetting = "same-line";
const OPTION_NEXT_LINE: BraceSetting = "next-line";

Choose a reason for hiding this comment

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

swap the order, reuse the constants.

const OPTION_SAME_LINE = "same-line";
const OPTION_NEXT_LINE = "next-line";
type BraceSetting = OPTION_SAME_LINE | OPTION_NEXT_LINE;

@johnwiseheart
Copy link
Contributor

Closing due to age and inactivity. Feel free to re-open or create a new pull request if you decide to continue working on this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants