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

Bump sourceror dependency to v0.12.2 #841

Closed
wants to merge 3 commits into from

Conversation

robmckinnon
Copy link

@robmckinnon robmckinnon commented Mar 9, 2023

Bump sourceror dependency to v0.12

  • As we discussed here this is a PR to bump sourceror to v0.12.
  • Sourceror v0.12 has some non-backwards compatible changes, particularly the removal of "ended" zippers.
  • I'm developin a refactor.extract.function code action that works with sourceror v0.12.
  • We specify sourceror version via a tag on its repo to ensure sourceror/lib_vendored is included and compilation in VSCode works.

Pass include_comments option to CodeMod.Ast.from/2

  • Use Sourceror.parse_string/1 wheninclude_comments: true option passed to CodeMod.Ast.from/2.
  • Add tests of CodeMod.Ast.from/2.

@robmckinnon robmckinnon changed the title Bump sourceror dependency to v0.12.1 WIP - Bump sourceror dependency to v0.12.1 Mar 9, 2023
@robmckinnon robmckinnon changed the title WIP - Bump sourceror dependency to v0.12.1 Bump sourceror dependency to v0.12.1 Mar 9, 2023
ElixirSense.string_to_quoted(s, 1, 6, token_metadata: true)
def from(s, opts) when is_binary(s) do
if opts[:include_comments] do
Sourceror.parse_string(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does sourceror fix errors like ElixirSense does?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know. I'm not familar with how ElixirSense fixes errors. Do you have examples or tests that check for this? We could extend those to test Sourceror's behaviour.

Comment on lines +29 to +41
test "from\1" do
assert {:ok,
{:def, [do: [line: 1, column: 16], end: [line: 3, column: 3], line: 1, column: 3],
[
{:foo, [closing: [line: 1, column: 14], line: 1, column: 7],
[{:baz, [line: 1, column: 11], nil}]},
[do: {:__block__, [], []}]
]}} =
Ast.from("""
def foo(baz) do
# TODO
end
""")
Copy link
Contributor

Choose a reason for hiding this comment

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

these assertions are pretty hard core, and are really testing the calls to elixir sense / sourceror

Perhaps instead, just ensure we're calling the two libs correctly in each case by using Patch?

Copy link
Author

Choose a reason for hiding this comment

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

The sourceror github page says to expect "frequent breaking changes".

Perhaps we leave the assertion of the returned AST structure -- at least for the Ast.from(_, include_comments: true) call -- to catch any breaking changes?

I depend on the AST returned by sourceror for the extract function refactoring code I'm writing in a separate branch.

@robmckinnon robmckinnon changed the title Bump sourceror dependency to v0.12.1 Bump sourceror dependency to v0.12.2 May 3, 2023
@lukaszsamson
Copy link
Collaborator

Sourceror library introduces problems with Mix.install and conflicts with sourceror versions used by client projects. If we want it we need it vendored like dialyxir and jason

@robmckinnon
Copy link
Author

Sourceror library introduces problems with Mix.install

@lukaszsamson Are you intending to remove {:sourceror, "0.11.2"} from the language_server mix file? https://github.com/elixir-lsp/elixir-ls/blob/master/apps/language_server/mix.exs#L38

When making this PR I found I needed to use tag to ensure lib_vendrored is included. Does that address the Mix.install issue you mentioned?

      # use tag to ensure sourceror/lib_vendored is included
      {:sourceror, github: "doorgan/sourceror", tag: "v0.12.2"},

and conflicts with sourceror versions used by client projects. If we want it we need it vendored like dialyxir and jason

Which client projects have the conflicts?

@lukaszsamson
Copy link
Collaborator

Are you intending to remove

Yes. It's not really used anywhere

When making this PR I found I needed to use tag to ensure lib_vendrored is included. Does that address the Mix.install issue you mentioned?

No idea TBH, but most likely no. A lot of people reported problems with build related to that dependency. Just search for sourceror in issues here/vsdode/forum/slack. And we need a proper vendoring. OTP has no code sandboxing. Whenever a client project has it as a dependency in some other version it will override modules from our dependencies.

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.

3 participants