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

Test apply-refact with TypeApplications #1244

Merged
merged 2 commits into from
Jan 22, 2021

Conversation

jneira
Copy link
Member

@jneira jneira commented Jan 21, 2021

@Ailrun
Copy link
Member

Ailrun commented Jan 22, 2021

Hm? It passes all tests?

@jneira
Copy link
Member Author

jneira commented Jan 22, 2021

@Ailrun yeah, i expected it, as it passes locally with master for me. But it failed in the past and TypeApplications is a delicate extension that modifies the syntax so i consider there is possible that there could be a regression

Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

LGTM :)


expectedTypeApp = [ "module ApplyRefact1 where", ""
, "a = id @Int 1"
]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change these tests to golden tests? If it sounds good, I'm willing to update it.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i was thinking in do it in the past. But otoh i suspect they will not change frequently and to have them close to the code has its value imo.
that said we are using golden tests in other tests and to be consistent is good too, so that change will welcomed, thanks!

@Ailrun Ailrun added the merge me Label to trigger pull request merge label Jan 22, 2021
@mergify mergify bot merged commit be754d9 into haskell:master Jan 22, 2021
@jneira jneira deleted the test-hlint-typeapp branch January 23, 2021 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants