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.

Sign up for GitHub

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

Jump to bottom

add more heuristics tests #4189

Merged
merged 1 commit into from
Jul 23, 2018
Merged

add more heuristics tests #4189

merged 1 commit into from
Jul 23, 2018

Conversation

smola
Copy link
Contributor

@smola smola commented Jul 9, 2018

Description

add more heuristics tests

@pchaigno
Copy link
Contributor

pchaigno commented Jul 9, 2018

Whoa! Thanks a lot @smola!

Is there any heuristic rules for which you didn't add a test? If not, maybe we should add a test to make sure every heuristic rule has a corresponding test in test_heuristics.rb---so as to not regress in future pull requests :-)

@smola
Copy link
Contributor Author

smola commented Jul 9, 2018

I think I did cover everything, but I have to double-check.

@smola smola force-pushed the heuristics-test branch from b7d8b1c to c43bb08 Compare July 23, 2018 09:02
@smola
Copy link
Contributor Author

smola commented Jul 23, 2018

.ml was missing, now it is complete.

@smola smola mentioned this pull request Jul 23, 2018
Merged
2 tasks
@pchaigno
Copy link
Contributor

Thanks @smola!

I'll try to find some time to add a pedantic test to check all heuristic rules have a corresponding test. I'd really prefer we merge it at the same time as this pull request, otherwise we might need to redo a similar pull request in a few months.

vmg
vmg approved these changes Jul 23, 2018
View reviewed changes
Copy link
Contributor

@vmg vmg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@pchaigno This PR is looking good. Have you found time to add the pedantic tests? If not, we could still merge this right away so #4087 can land.

pchaigno
pchaigno approved these changes Jul 23, 2018
View reviewed changes
Copy link
Contributor

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

@vmg That was an hour ago. Let's merge if nobody else has time.

@vmg
Copy link
Contributor

vmg commented Jul 23, 2018

Ah shit, sorry, I just assumed this was a stale issue. Monday morning 😪

@vmg vmg merged commit 51feda9 into github-linguist:master Jul 23, 2018
@pchaigno pchaigno mentioned this pull request Jul 23, 2018
Closed
@smola smola deleted the heuristics-test branch July 23, 2018 15:33
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@vmg vmg vmg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smola @pchaigno @vmg