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

#188, but fixed #191

Merged
merged 10 commits into from
Aug 26, 2021
Merged

#188, but fixed #191

merged 10 commits into from
Aug 26, 2021

Conversation

pfitzseb
Copy link
Member

@pfitzseb pfitzseb commented Aug 25, 2021

This fixes the simple_hash function introduced in #188. The new implementation just pushes all characters into an Int128 (so simple_hash("if") == 508 because 'f'-'a' == 05 and 'i'-'a' == 08), which works fine because keywords are at most 10 characters long at the moment.

Fixes #189.

@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #191 (e8795c7) into master (74f8b87) will decrease coverage by 1.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
- Coverage   78.83%   77.65%   -1.18%     
==========================================
  Files           4        4              
  Lines        1063      846     -217     
==========================================
- Hits          838      657     -181     
+ Misses        225      189      -36     
Impacted Files Coverage Δ
src/_precompile.jl 0.00% <ø> (ø)
src/lexer.jl 93.62% <100.00%> (+1.33%) ⬆️

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 74f8b87...e8795c7. Read the comment docs.

@KristofferC
Copy link
Sponsor Member

Would it be easy to add a test that ensures this is ok?

@pfitzseb
Copy link
Member Author

pfitzseb commented Aug 25, 2021

Added a test that goes over all lower-case ASCII strings up to length 5. Takes a minute or two, but that should be fine. Of course it doesn't really test correctness though... It did find a bug in my code, so that's good I guess :P

@KristofferC
Copy link
Sponsor Member

@odow, would it be possible for you to manually test this PR and see if this is ok w.r.t the issues you had with the earlier one?

@domluna
Copy link

domluna commented Aug 25, 2021

I can update the compat for the formatter once this lands

@KristofferC
Copy link
Sponsor Member

I don't see how the compat for JuliaFormatter is related here (since the buggy PR got reverted and a new patch version tagged)

@domluna
Copy link

domluna commented Aug 25, 2021

hmm maybe not required since if they update or fresh install it would get the more recent version of the dependency right?

@KristofferC
Copy link
Sponsor Member

Yes, and we should yank the buggy one for completeness sake.

@odow
Copy link

odow commented Aug 25, 2021

Confirmed that it works:

julia> using JuliaFormatter

julia> format("src/lp_sensitivity.jl")
true

(@v1.6) pkg> st
      Status `~/.julia/environments/v1.6/Project.toml`
  [98e50ef6] JuliaFormatter v0.15.8
  [0796e94c] Tokenize v0.5.18 `https://github.com/JuliaLang/Tokenize.jl.git#load-time`

@KristofferC KristofferC merged commit c4027ef into master Aug 26, 2021
@KristofferC KristofferC deleted the load-time branch August 26, 2021 07:36
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.

Tokenize 0.5.19 broke JuMPs formatting
5 participants