Closed FelipeLema closed 5 years ago
Need help with REQUIRES
→ Project.toml
. I have no idea what to do and the packages I've looked into (namely Dataframes.jl) have no info of such a migration.
Thanks for this effort! I'll take some time to review this weekend or early next week. REQUIRES to Project.toml change is not mandatory. I think we can leave it for a future PR.
Oh, BTW I'm removing code for supporting anything previous to to v1.0
We'll need to update the .travis.yml
file also. Try something like: https://github.com/JuliaCI/TravisTest.jl/blob/master/.travis.yml except without the versions that won't be supported.
Also, would you mind if I added https://github.com/kmsquire/Match.jl
as a dependency? With it I could get rid of all those if … else if… else if…
and make the code much more easy to read.
Easy-to-read is important :+1:.
By the way, --compile-cache=no
is now --compiled-modules=no
. See https://github.com/TotalVerb/EnglishText.jl/blob/master/.travis.yml for an example.
Is there a particular reason why you're testing against the message when linting? Isn't the code enough?
On another matter, I've bumped into this :(#= none:1 =#)
again and I don't what its purpose, what we should do with it or even where to get more information about it.
let ex= Meta.parse("f() = x")
@show ex
@show ex.head
@show ex.args[2].head
@show ex.args[2].args[1]
end
Prints the following
ex = :(f() = begin
#= none:1 =#
x
end)
ex.head = :(=)
(ex.args[2]).head = :block
(ex.args[2]).args[1] = :(#= none:1 =#)
It's a LineNumberNode
. The Julia parser produces these to keep track of line numbers, which are needed for reporting things like line numbers for errors. I'm not sure if Lint uses this information right now.
As for testing against the message, it's probably enough to test against the error code. Historically we did not have error codes, and so we tested only against the message.
was LineNumberNode
introduced in v0.7 or later? just so I keep an eye out
I think it was already around by 0.6, but I'm not sure.
We will probably need to add Test
to test/REQUIRE
? Just trying to interpret the travis error.
I would say the other way around: remove Test
from REQUIRE
.
Isn't Test
part of Julia's std lib? If so, should it be listed in REQUIRE
?
You are probably right. I'm not sure how to interpret this test failure then.
The :I342
unit test used exported Base.var
, but this was moved to Statistics.var
.
The thing is that I'm not sure if it's still broken or if I'm not using a proper replacement. I tried Base.round
and Base.ndigits
, but it pops out both :I343
and :I342
.
Should it be enough that we test that the issue is among all the reported issues? I've seen assertions on single issue reporting, so I though this might also be the case.
nevermind. Using +
the test case passes
I don't think we should have W443 anymore. Docile.jl has been deprecated
I don't think W443 has anything to do with Docile. The aim is to catch code like
@doc "this is a test"
f() = 0
which does not document function f
. Perhaps what was intended was
@doc "this is a test" ->
f() = 0
That said, in the interest of getting things working and reducing feature bloat, I don't mind if you remove W443. I think in the future someone can work on a more general "this statement appears to do nothing" lint rule, which will catch this obscure case.
Oh, I was reading it the other way around.
It's not that terrible to leave it there. Parsing has added LineNumberNode
s in unexpected places, messing up tests against length((expression::Expr).args)
.
I'll filter out this nodes, since it's something we'll eventually have to do here or in other places.
There are a bunch of stuff I want to do, having no length(ex.args)
as conditions anywhere in the code is one of them. But let's stabilize the package first.
There's a @test_broken
in test/curly.jl. Is this supposed to be "fixed sometime, currently WIP"? Test
complains it passes
I think it's a test that's supposed to pass but wasn't because of a bug, so it can be turned back into @test
now.
As you can see in the test/variables.jl
file I just uploaded, there is a crucial part of the package working incorrectly. There is still some other bits that we can get along without (the @doc
stuff), but without solving this test, there will be false positives all over the place.
The exact test fails @ https://travis-ci.org/tonyhffong/Lint.jl/jobs/470803598#L528
Now I'm OK with this being merged (182/200 test cases pass)
I had originally broken the parsing. It's unfortunate that I re-wrote it but it ended up being just as awful.
Now string -> expression code is more organized. At least now it's got its own unit tests.
Go ahead with the merge if you're OK with it.
Thanks! The next problem is how to release this package, since attobot is not installed. We may need to make a METADATA PR manually, but I have not done that in so long.
I have never done such a thing 😖
Have a few fixes so far, but still incomplete. I made this PR for visibility so anyone can contribute
Will ping when this is done (unit tests pass).