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

Conversation

jamescdavis
Copy link
Contributor

@jamescdavis jamescdavis commented Feb 28, 2018

PR checklist

Overview of change:

Add a match-declaration-order-only option to object-literal-sort-keys that behaves the same as match-declaration-order but does not fall back to checking alphabetical sorting when no contextual type is is found.

CHANGELOG.md entry:

[new-rule-option]: object-literal-sort-keys: Add match-declaration-order-only option

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @jamescdavis! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

},
optionExamples: [
true,
[true, OPTION_IGNORE_CASE, OPTION_MATCH_DECLARATION_ORDER, OPTION_SHORTHAND_FIRST],
[true, OPTION_IGNORE_CASE, OPTION_MATCH_DECLARATION_ORDER, OPTION_MATCH_DECLARATION_ORDER_ONLY, OPTION_SHORTHAND_FIRST],

Choose a reason for hiding this comment

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

add a new option example with this new option and not OPTION_MATCH_DECLARATION_ORDER. let's not encourage users to set both.

@jamescdavis jamescdavis force-pushed the add_match-declaration-order-only_option_to_object-literal-sort-keys branch from f85ecf9 to 246e48f Compare May 3, 2018 13:25
@giladgray
Copy link

@jamescdavis 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.

@jamescdavis jamescdavis force-pushed the add_match-declaration-order-only_option_to_object-literal-sort-keys branch from 246e48f to 2b4306e Compare August 15, 2018 05:59
@jamescdavis
Copy link
Contributor Author

updated

but don't fall back to alphabetical if a contextual type is not found.

Note: If both ${OPTION_MATCH_DECLARATION_ORDER_ONLY} and ${OPTION_MATCH_DECLARATION_ORDER} options are present,
${OPTION_MATCH_DECLARATION_ORDER_ONLY} will take precedence and alphabetical fallback will not occur.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about console.warning instead of explicitly mentioning it in the docs? Feels like this is an error case that we shouldn't allow.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

updated with @JoshuaKGoldberg's suggestion and fixed merge conflicts

@jamescdavis
Copy link
Contributor Author

Thanks for fixing this up, @adidahiya!

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

Successfully merging this pull request may close these issues.

5 participants