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

[analyzer] support all default param types #160

Merged
merged 4 commits into from
Apr 18, 2018

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Apr 17, 2018

Fixes #132.

We were only accepting literal right hand values before, but really i suppose we don't care what it is. it gets turned into a string anyway.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Deleting code AND fix issues. FeelsGoodMan! I wanted to take a stab, but this solution is the best 😄

Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

Nice!

@rictic
Copy link
Contributor

rictic commented Apr 17, 2018

master was broken when you sent this over, mind rebasing real quick?

@rictic
Copy link
Contributor

rictic commented Apr 18, 2018

Pushed up a merge commit, if it passes I'll merge this in

@43081j
Copy link
Contributor Author

43081j commented Apr 18, 2018

thanks for that, had gone mobile yesterday 🙊

the tests failed because of an ordering mistake in the assertions by the looks of it. will fix it shortly

@rictic
Copy link
Contributor

rictic commented Apr 18, 2018

LGTM!

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

Successfully merging this pull request may close these issues.

4 participants