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

Restricted Linter options type #1168

Merged
merged 9 commits into from
Apr 28, 2016
Merged

Restricted Linter options type #1168

merged 9 commits into from
Apr 28, 2016

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Apr 26, 2016

Fixes #1167

Wasn't sure where unit tests for this would go, if you do want to test constructor logic.

@jkillian
Copy link
Contributor

👍 We can probably get away without a test here for now, it would break some of the full CLI tests if it didn't accept valid input. Do you think we should also add a check for == null and throw an error then?

@JoshuaKGoldberg
Copy link
Contributor Author

Yeah, that sounds like a good thing to add.

Do you consider any option fields mandatory? If so, instead of checking for typeof it might make sense to check for their existence.

@jkillian
Copy link
Contributor

Strangely enough, it looks like ILinterOptions.formatter must be specified, while everything else can be left undefined. This seems more like a bug to me than anything, so let's not worry about it - I think ILinterOptions.formatter should be made optional in the long run.

On the other hand, if everything is optional, passing null as the current options isn't unreasonable. Maybe we should just modify computeFullOptions to handle the case where the whole options object is null and to provided a default value for formatter if not provided?

@JoshuaKGoldberg
Copy link
Contributor Author

Thoughts on this draft? Is there some source of truth for what formattershould default to?

return {
configuration: configuration || DEFAULT_CONFIG,
formatter: formatter || "json",
formattersDirectory: formattersDirectory || "",
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not add the || "" here, it's fine if this stays null. (An empty string would get treated as the current directory which could lead to unexpected behavior)

@jkillian
Copy link
Contributor

Left my thoughts above @JoshuaKGoldberg, feel free to disagree on any of them!

@jkillian
Copy link
Contributor

Beautiful! This turn out very nice

@jkillian jkillian merged commit 62a57af into palantir:master Apr 28, 2016
@JoshuaKGoldberg JoshuaKGoldberg deleted the linter-options-type branch April 28, 2016 19:07
tomduncalf pushed a commit to tomduncalf/tslint that referenced this pull request Jun 14, 2016
Also provide sane defaults for all linter configuration options.
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.

3 participants