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

Get Lint working on 0.6 #172

Merged
merged 4 commits into from
Feb 7, 2017
Merged

Get Lint working on 0.6 #172

merged 4 commits into from
Feb 7, 2017

Conversation

TotalVerb
Copy link
Collaborator

@TotalVerb TotalVerb commented Jan 20, 2017

The first commit converts DataType to Type everywhere. This should always have been the case, but is especially important in 0.6, as many types like Array are now UnionAlls instead of DataTypes.

The second commit rewrites the version constraint code to not call eval. Not calling eval is a good idea anyway, but is especially important in 0.6 due to the world age restrictions. The calling of new methods created with eval is subject to world age restrictions in 0.6, and removing the eval addresses that issue.

The third commit changes from typeof(x) == T to isa(x, T) in many places. 0.6 introduces new x isa T syntax which would make sense to use in the future, and this is a first step toward that modernization.

The fourth commit addresses the remaining, miscellaneous problems that prevent Lint from working properly on 0.6, and involves some updates to legacy code (this fixes several issues along the way). Overall, Lint should be better at detecting types. Once 0.4 support is dropped, large amounts of code can be removed.

The fifth commit (in progress) fixes breakage of 0.4 and 0.5 introduced by the first four commits.

Fixes #79
Fixes #137

This was referenced Jan 20, 2017
@coveralls
Copy link

coveralls commented Jan 20, 2017

Coverage Status

Coverage decreased (-1.8%) to 86.272% when pulling 485bf81 on TotalVerb:fw/fix-0.6 into 9028f4f on tonyhffong:master.

@TotalVerb TotalVerb changed the title Get Lint working on 0.6 WIP: Get Lint working on 0.6 Jan 20, 2017
@coveralls
Copy link

coveralls commented Jan 20, 2017

Coverage Status

Coverage decreased (-1.8%) to 86.272% when pulling d38b027 on TotalVerb:fw/fix-0.6 into 9028f4f on tonyhffong:master.

@coveralls
Copy link

coveralls commented Jan 20, 2017

Coverage Status

Coverage decreased (-1.8%) to 86.272% when pulling d05da1b on TotalVerb:fw/fix-0.6 into 9028f4f on tonyhffong:master.

@TotalVerb
Copy link
Collaborator Author

TotalVerb commented Feb 5, 2017

There was some code incorrectly supposing that slicedim always returns either a vector or a matrix, when in fact this depends on the values of the arguments. Removing that code (and test) is necessary for the tests to pass on 0.6, where inference is used to guess the result type instead.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 86.024% when pulling d7ac0cd on TotalVerb:fw/fix-0.6 into b0005ca on tonyhffong:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 86.024% when pulling d7ac0cd on TotalVerb:fw/fix-0.6 into b0005ca on tonyhffong:master.

@TotalVerb TotalVerb changed the title WIP: Get Lint working on 0.6 Get Lint working on 0.6 Feb 5, 2017
@TotalVerb
Copy link
Collaborator Author

All the tests are passing and I think this is ready to go after review!

@tonyhffong
@Michael-Klassen

Once this is in I'll have another PR fixing up minor issues and cleaning up some loose ends.

@@ -148,7 +148,7 @@ end
msgs = lintstr(s)
@test msgs[1].code == :E521
@test msgs[1].variable == "a"
@test contains(msgs[1].message, "apparent type DataType is not a container type")
@test contains(msgs[1].message, "apparent type Type")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep the wording on this error message or change it to be more descriptive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The error message is actually the same. I had to change the test because it prints as Type{T} on 0.5 or Type on 0.6, and I didn't want to select which based off version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think, going forward, we will want to standardize the error messages based off error codes, and then disassociate the testing of linting creating the right error code, and testing the error codes being associated with an appropriately descriptive message.

@Michael-Klassen Michael-Klassen merged commit 4243463 into tonyhffong:master Feb 7, 2017
@TotalVerb TotalVerb deleted the fw/fix-0.6 branch February 7, 2017 00:42
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.

False positive when constructor possibly raises error Incorrect type assumption with int()
3 participants