Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_service): correctly send file source to workspace #4466

Merged
merged 3 commits into from
May 12, 2023

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented May 12, 2023

Summary

Closes #4458

The previous PR was send the default SourceType, which wasn't the correct way.

This PR fixes the issue by doing the following:

  • renaming "source type" to "file source";
  • creating a tiny trait called FileSource, which will be used to share behaviour for those file sources that need to be sent across thread boundaries;
  • creating a new type called AnyFileSource, a language-agnostic data structure that will be used when using the Workspace. This type has a method called try_from, which will attempt to recreate the file source based on the path of the file

I renamed SouceType to JsFileSource.

Test Plan

Current tests should pass. I created a new test case to fix the issue

Changelog

  • The PR requires a changelog line

(I will create changelog line in next-release branch, to avoid too many conflicts)

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented May 12, 2023

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit 49bd68f
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/645e7bdcdd09d6000810ec5f

@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core A-Formatter Area: formatter A-Linter Area: linter A-Parser Area: parser A-Project Area: project configuration and loading A-Tooling Area: our own build, development, and release tooling labels May 12, 2023
@github-actions
Copy link

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 48863 48863 0
Passed 47796 47796 0
Failed 1067 1067 0
Panics 0 0 0
Coverage 97.82% 97.82% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6212 6212 0
Passed 1763 1763 0
Failed 4449 4449 0
Panics 0 0 0
Coverage 28.38% 28.38% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 573 573 0
Failed 66 66 0
Panics 0 0 0
Coverage 89.67% 89.67% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17224 17224 0
Passed 13127 13127 0
Failed 4097 4097 0
Panics 0 0 0
Coverage 76.21% 76.21% 0.00%

@ematipico ematipico force-pushed the fix/source-type-linter branch 2 times, most recently from eb106df to ad09824 Compare May 12, 2023 16:09
@ematipico
Copy link
Contributor Author

Turns out we can't use a unwrap_cast, but we have to gracefully handle the error. Via LSP, we can get documents that don't have extensions, and those should not throw a panic.

@ematipico ematipico merged commit 5771e8b into main May 12, 2023
@ematipico ematipico deleted the fix/source-type-linter branch May 12, 2023 18:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI A-Core Area: core A-Formatter Area: formatter A-Linter Area: linter A-Parser Area: parser A-Project Area: project configuration and loading A-Tooling Area: our own build, development, and release tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 noUndeclaredVariables reports TypeScript utility types
2 participants