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

Restore compatibility with Julia 1.1 #22

Merged
merged 4 commits into from Mar 29, 2019

Conversation

Projects
None yet
4 participants
@tribut
Copy link
Contributor

commented Mar 28, 2019

This commit resolves the following issues:

  • UUIDs were used as keys for a Dict{String,Any}, leading to an exception being thrown.
  • depot["manifest"] is now indexed by UUID instead of by name.
  • nullfile was never closed (not strictly an issue with Julia 1.1, but the fact that output redirection was not restored lead to the silent failure / freeze with Julia 1.1)

Fixes JuliaEditorSupport/julia-vscode#647

@tribut tribut force-pushed the tribut:julia-1.1 branch from 263a005 to f1d9137 Mar 28, 2019

@tribut tribut referenced this pull request Mar 28, 2019

Closed

julia 1.1 compatability #647

@davidanthoff
Copy link
Contributor

left a comment

Yeah, thanks so much for taking a stab!

I think this PR mixes the order of name and uuid up. Not 100% sure, but pretty convinced of that :)


function pkg_uuid(pkg)
return string(last(pkg))
end

This comment has been minimized.

Copy link
@davidanthoff

davidanthoff Mar 28, 2019

Contributor

I think it is just the other way around, i.e. isn't the first element in the Pair the uuid and the last one the name?

if haskey(depot["manifest"], first(pkg))
for pkg1 in depot["manifest"][first(pkg)]
if pkg1["uuid"] == last(pkg)
if haskey(depot["manifest"], pkg_name(pkg))

This comment has been minimized.

Copy link
@davidanthoff

davidanthoff Mar 28, 2019

Contributor

I'm 90% sure that the dict in depot["manifest"] is uuid->name.

So I think we also need to make sure that when we import that dict, that we convert the uuids on julia 1.1. to strings in that dict, otherwise the key lookup won't work. I think that probably needs to be done here.

I think that was the general problem when I looked last: we have a bunch of dicts where the key type is string on julia 1.0 but UUID on julia 1.1, because we just get those dicts directly from Pkg. I think we need to convert all those dicts to string->whatever for it to work with both julia 1.0 and 1.1

tribut added some commits Mar 28, 2019

Convert UUIDs to Strings on import
Also refactor "create depot" code into function.
Fix compatibility with Julia 1.1
depot["manifest"] used to have the package names as keys. The new
manifest uses UUIDs.

Before, UUIDs were just strings - they are now a special type, so we
have to convert when accessing the Dicts (throws a KeyError otherwise).

@tribut tribut force-pushed the tribut:julia-1.1 branch from f1d9137 to 767e4e3 Mar 28, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Mar 28, 2019

Codecov Report

Merging #22 into master will decrease coverage by 0.33%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #22      +/-   ##
=========================================
- Coverage    4.87%   4.54%   -0.34%     
=========================================
  Files           3       3              
  Lines         123     132       +9     
=========================================
  Hits            6       6              
- Misses        117     126       +9
Impacted Files Coverage Δ
src/clientprocess/clientprocess_main.jl 0% <0%> (ø) ⬆️
src/clientprocess/from_static_lint.jl 0% <0%> (ø) ⬆️
src/SymbolServer.jl 10.9% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa5fa7e...767e4e3. Read the comment docs.

2 similar comments
@codecov-io

This comment has been minimized.

Copy link

commented Mar 28, 2019

Codecov Report

Merging #22 into master will decrease coverage by 0.33%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #22      +/-   ##
=========================================
- Coverage    4.87%   4.54%   -0.34%     
=========================================
  Files           3       3              
  Lines         123     132       +9     
=========================================
  Hits            6       6              
- Misses        117     126       +9
Impacted Files Coverage Δ
src/clientprocess/clientprocess_main.jl 0% <0%> (ø) ⬆️
src/clientprocess/from_static_lint.jl 0% <0%> (ø) ⬆️
src/SymbolServer.jl 10.9% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa5fa7e...767e4e3. Read the comment docs.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 28, 2019

Codecov Report

Merging #22 into master will decrease coverage by 0.33%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #22      +/-   ##
=========================================
- Coverage    4.87%   4.54%   -0.34%     
=========================================
  Files           3       3              
  Lines         123     132       +9     
=========================================
  Hits            6       6              
- Misses        117     126       +9
Impacted Files Coverage Δ
src/clientprocess/clientprocess_main.jl 0% <0%> (ø) ⬆️
src/clientprocess/from_static_lint.jl 0% <0%> (ø) ⬆️
src/SymbolServer.jl 10.9% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa5fa7e...767e4e3. Read the comment docs.

@tribut

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

As discussed on slack, in addition to UUIDs no longer being Strings, the way the manifest was indexed also changed (old: names, new: uuid). This also explains the confusion in the first version of this PR.

@davidanthoff davidanthoff merged commit be60a94 into JuliaEditorSupport:master Mar 29, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidanthoff

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Alright, fantastic! I reviewed it and it looks good, and I also think we should just cut a beta/alpha release of the extension and ask folks to try this out in the wild.

This was referenced Mar 29, 2019

@polypus74

This comment has been minimized.

Copy link

commented Apr 12, 2019

Any progress on this by any chance?

@davidanthoff

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

Yes, this shipped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.