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

Fix suggest contracts windows regression #531

Merged
merged 5 commits into from
Apr 26, 2021
Merged

Conversation

lukaszsamson
Copy link
Collaborator

No description provided.

on windows we need to convert from "c:\\some\\other.ex" to "c:/some/other.ex"

Fixes #528
Copy link
Member

@axelson axelson left a comment

Choose a reason for hiding this comment

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

Do you understand what caused this regression? We haven't touched this portion of the code in a while. And it's not like we upgraded dialyxir either.

I'm also thinking we should create SourceFile.abs_path_from_uri since we're calling Path.absname on the output so often.

@lukaszsamson
Copy link
Collaborator Author

I guess it worked on Windows purely by chance and got broken when I did the URI parsing refactoring and fixes.

I'm also thinking we should create SourceFile.abs_path_from_uri since we're calling Path.absname on the output so often.

Good idea. Besides that we'd need some test coverage on suggest specs. I'll add those in this PR later.
It would also help if we had Windows CI pipeline.

@lukaszsamson lukaszsamson requested a review from axelson April 25, 2021 19:41
Copy link
Member

@axelson axelson left a comment

Choose a reason for hiding this comment

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

❤️

@axelson axelson merged commit 1102cff into master Apr 26, 2021
@axelson axelson deleted the ls-fix-windows-regression branch April 26, 2021 00:40
vanjabucic pushed a commit to vanjabucic/elixir-ls that referenced this pull request Jul 5, 2021
* Dialyzer.suggest_contracts expects absnames

on windows we need to convert from "c:\\some\\other.ex" to "c:/some/other.ex"

Fixes elixir-lsp#528

* fix test on windows

* add suggest contracts tests

* apply code review suggestion

* revert non windows compatible change
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.

2 participants