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

Updates for Julia 1.0 #41

Merged
merged 8 commits into from
Aug 27, 2018
Merged

Updates for Julia 1.0 #41

merged 8 commits into from
Aug 27, 2018

Conversation

wookay
Copy link
Contributor

@wookay wookay commented Aug 10, 2018

hi. fixed at darwin room.

src/LibPQ.jl Outdated
@@ -7,15 +7,15 @@ using Compat.Dates
using DocStringExtensions
using Decimals
using DataStreams
using Base.Iterators: zip, product
using IterTools: imap
import Base.Iterators: zip, product
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this? Since these functions aren't being overloaded, using is appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. just understood that how to import and using. good time in the hackathon!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder that which one makes this warning on the ci.

┌ Warning: `using A: B` will only be allowed for single bindings, not modules. Use `using A.B` instead
│   caller = ip:0x0
└ @ Core :-1

https://ci.appveyor.com/project/invenia/libpq-jl/build/job/4d449u0l8khpd2il#L148

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, there's also Julia issue #25397 . I don't know much.

Copy link
Collaborator

@iamed2 iamed2 left a comment

Choose a reason for hiding this comment

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

Should also upgrade the travis script to test on 0.6, 0.7, 1.0, and nightly

src/LibPQ.jl Outdated
@@ -7,15 +7,15 @@ using Compat.Dates
using DocStringExtensions
using Decimals
using DataStreams
using Base.Iterators: zip, product
using IterTools: imap
import Base.Iterators: zip, product
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use using and not import for all of these, including Memento (although the explicit imports are fine)

src/parsing.jl Outdated
return (sv, start(sv))
end
if VERSION >= v"0.7.0-DEV.5125"
Base.iterate(pqv::PQValue, i=1) = iterate(string_view(pqv), i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no guarantee that the first state of string iteration is 1, so iterate(pqv::PQValue) should pull it from iterate(string_view(pqv)). Same is true of the other iteration methods.

@wookay
Copy link
Contributor Author

wookay commented Aug 11, 2018

yahhh, I'm going to fix the LayerDicts.jl for 1.0

@JuliaDatabases JuliaDatabases deleted a comment from wookay Aug 11, 2018
src/parsing.jl Outdated
end
if VERSION >= v"0.7.0-DEV.5125"
Base.iterate(pqv::PQValue) = iterate(string_view(pqv))
Base.iterate(pqv::PQValue, i) = iterate(string_view(pqv), i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This creates string_view each time; it should be passed around as part of the state as in the old iteration functions below. That should only hold for PQValue, not the other iterators though.

src/parsing.jl Outdated
iterate(pqv, (sv, firstindex(sv)))
end
function Base.iterate(pqv::PQValue, (sv, sv_state))
iterate(sv, sv_state)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This generates just the item and the state for sv, but it has to put sv back in the state that it returns

src/parsing.jl Outdated
@@ -118,7 +118,10 @@ if VERSION >= v"0.7.0-DEV.5125"
iterate(pqv, (sv, firstindex(sv)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, missed this last time, but you need to actually iterate sv in this function; firstindex is not guaranteed to be the iteration state.

@wookay
Copy link
Contributor Author

wookay commented Aug 11, 2018

oh, rebased totally wrong...
rebased..

- iterate PQValue, PQTypeMap, PQConversions
- add 1.0 to the CIs
@ExpandingMan
Copy link
Contributor

ExpandingMan commented Aug 15, 2018

Note that your scripts are failing on 1.0 because of package management deprecations. You can see an example of some updated after_success lines here.

@iamed2
Copy link
Collaborator

iamed2 commented Aug 15, 2018

@wookay are you interested in fixing those or shall I?

@wookay
Copy link
Contributor Author

wookay commented Aug 16, 2018

@ExpandingMan , @iamed2 thanks. please fix anytime that you want. and It failed on LayerDicts.jl in Julia 1.0.

@ExpandingMan
Copy link
Contributor

LayerDicts seems fixed, any chance we can merge that and re-run this? It would be a shame to have to use forks of LibPQ, it seems in pretty good shape.

@iamed2
Copy link
Collaborator

iamed2 commented Aug 23, 2018

That's my to-do to-day

@iamed2
Copy link
Collaborator

iamed2 commented Aug 26, 2018

It's been taking a while to get tagged versions of dependencies but I think we're nearly there.

@iamed2 iamed2 merged commit ffa8298 into JuliaDatabases:master Aug 27, 2018
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.

5 participants