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

rename refactor #202

Merged
merged 19 commits into from
Oct 23, 2019
Merged

rename refactor #202

merged 19 commits into from
Oct 23, 2019

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Oct 23, 2019

UPDATED: moved to #203


Purpose

Implement rename refactoring command:

  • rename local binding
  • rename global bindings (module-based)

Approach

  • use MacroTools.jl for easy tokenizing and code replacing
  • for module-based global refactoring, CSTParser-based static file detection is used:
    • since our Revise-like module file detection will easily fail with the refactorings, which breaks precompilation cache
  • first tries local refactoring, and then move to global refactoring if needed
  • rename refactoring on fields is all suppressed, since not every code is statically typed and so I think we would end up with lots of false positive
  • global refactoring are designed to work only when it's called on the definition place

Task / Achievements

  • local refactor
  • module-based global refactor
  • graceful messages
    • notice the need of refactoring in the parent module if the refactored binding is imported/overloaded
    • report back error message
    • notice refactored files
    • show context
  • handle edge cases
    • suppress keyword refactoring
    • suppress field refactoring
    • suppress global refactoring that are not called at a definition place
    • halt global refactoring if files without write permission detected
    • global refactoring with dot accessor like Atom.isdebugging(), Base.countlines(args...)
    • global refactoring on macro
  • test

@aviatesk
Copy link
Member Author

@aviatesk
Copy link
Member Author

woa, travis has been revived ! :)

@codecov
Copy link

codecov bot commented Oct 23, 2019

Codecov Report

Merging #202 into master will decrease coverage by 2.04%.
The diff coverage is 12.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
- Coverage   39.52%   37.48%   -2.05%     
==========================================
  Files          36       37       +1     
  Lines        1824     1934     +110     
==========================================
+ Hits          721      725       +4     
- Misses       1103     1209     +106
Impacted Files Coverage Δ
src/Atom.jl 100% <ø> (ø) ⬆️
src/refactor.jl 0% <0%> (ø)
src/docs.jl 0% <0%> (ø) ⬆️
src/utils.jl 85.54% <37.93%> (-9.82%) ⬇️
src/completions.jl 76.99% <80%> (ø) ⬆️

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 117a5f4...b8802f3. Read the comment docs.

catch err
return Dict(:error => errdescription(old, new, err))
end
end
Copy link
Member Author

Choose a reason for hiding this comment

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

I agree the logic here would look messy, so please feel free to ask me if you have anything unsure -- I probably miss some edge cases.

@aviatesk
Copy link
Member Author

aviatesk commented Oct 23, 2019

Here are some edge cases I've noticed but still I'm not sure about how to handle.

local refactoring

If we have the code like:

function outer()
  val = 10
  function inner(val)
    2 * val
  end
  return inner(val)
end

and we refactor on outer's val, the inner's vals are also refactored, even though we may want to only refactor outers.
This is obviously because the inner function is included in the outer function scope, but I still not found any good way to handle this case with MacroTools's source walk.
As an alternative, this case would be easily solved if we can directly manipulate CSTParser's expressions and then reconstruct the source appropriately. But I'm not sure even if it's possible.

global refactoring

If a module contains code like:

function fun() end

function other()
  fun = 10
end

and we rename fun to fun2, then the binding fun in other will also be renamed to fun2. This is because I intentionally looks just for fun tokens, not for something more complicated like call sites (the code here), since in Julia every binding is an object, and we can have a code like map(fun, itr).
Current behavior can be annoying, but at least doesn't break code. If we just looks for call sites, then we may have false negatives, which would break the code. So imho the current behavior may suffice.

@aviatesk aviatesk merged commit b8802f3 into master Oct 23, 2019
@aviatesk
Copy link
Member Author

Damn, I missed to push to the master ! Will try to revert all the commits.

@aviatesk
Copy link
Member Author

Okay, sorry for the confusing.
283d466 (on master) successfully reverted all the commits in this PR that were mistakenly pushed to master.
And this PR is moved into #203.

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.

1 participant