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

Add auto-fix for DropConsOfItemAndList #140

Merged
merged 5 commits into from
Jan 1, 2018

Conversation

adeschamps
Copy link
Contributor

Another box on #5

I also modified FileContent.updateRange to treat ranges as [inclusive, exclusive), which feels more natural - for example, if you use the end of one range as the start of another, then they don't overlap.

Copy link
Owner

@stil4m stil4m left a comment

Choose a reason for hiding this comment

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

Looks good! A few small changes. Thanks for your work :)

, schema =
Schema.schema
|> Schema.rangeProp "range"
|> Schema.rangeProp "car"
|> Schema.rangeProp "cdr"
Copy link
Owner

Choose a reason for hiding this comment

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

What do car and cdr mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, those are Lisp-y terms that crept in. car and cdr are analogous to List.head and List.tail. I should probably use head and tail instead.

fix : ( String, File ) -> MessageData -> Result String String
fix ( content, _ ) messageData =
case
( Data.getRange "car" messageData
Copy link
Owner

Choose a reason for hiding this comment

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

Use a Maybe.map2 (,) to compose the two Maybes into 1. Try to reduce the use of _ as much as possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's much cleaner.

fixContent carRange cdrRange content |> Ok

_ ->
Err "Invalid message data for fixer UnnecessaryParens"
Copy link
Owner

Choose a reason for hiding this comment

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

'UnnecessaryParens': Message is copied from somewhere else. Please use the correct check name. (I've created an issue to fix this #141)

- Replace "car" and "cdr" with "head" and "tail"
- Tidy up match expression
- Fix copy-paste mistake in error message
@adeschamps
Copy link
Contributor Author

I made a few changes. I think it's all set.

Copy link
Owner

@stil4m stil4m left a comment

Choose a reason for hiding this comment

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

Approved (with one small change left)

@@ -42,7 +44,7 @@ scan fileContext _ =
onExpression : Ranged Expression -> Context -> Context
onExpression ( r, inner ) context =
case inner of
OperatorApplication "::" _ _ ( _, ListExpr _ ) ->
OperatorApplication "::" _ ( carRange, _ ) ( cdrRange, ListExpr _ ) ->
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe rename these to headRange and tailRange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I thought I had gotten all of them.

@adeschamps
Copy link
Contributor Author

All set

@stil4m stil4m merged commit bec7fbe into stil4m:dev Jan 1, 2018
@stil4m stil4m added this to the 0.14.0 milestone Jan 1, 2018
@adeschamps adeschamps deleted the drop-cons-of-item-and-list branch January 1, 2018 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants