Let LocalFileLinter reuse the Dependency instead falling back on its path
#3586
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JCZuurmond
added
migrate/code
JCZuurmond
temporarily deployed
to
account-admin
GitHub Actions
Inactive
|
✅ 57/57 passed, 2 skipped, 25m39s total Running from acceptance #8171 |
JCZuurmond
temporarily deployed
to
account-admin
GitHub Actions
Inactive
JCZuurmond
force-pushed
the
fix/update-file-linter-signature
branch
from
7447e12 to
c58925d
Compare
JCZuurmond
temporarily deployed
to
account-admin
GitHub Actions
Inactive
JCZuurmond
temporarily deployed
to
account-admin
GitHub Actions
Inactive
pritishpai
reviewed
Feb 4, 2025
| problems[idx] = dataclasses.replace(problem, source_path=self._path) | ||
| return problems | ||
| # supported language that does not generate dependencies | ||
| if self.language == Language.SQL: |
There was a problem hiding this comment.
Can we combine this and the other return as SQL is also unsupported and probably doesn't need an if statement
There was a problem hiding this comment.
I prefer to keep it like this because SQL is supported, it just does not return a dependency graph, see #3639
pritishpai
approved these changes
Feb 4, 2025
2 tasks
This was referenced Feb 5, 2025
|
Surpassed by #3641 |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 5, 2025
## Changes Use `Language` on `LocalFile`, not `CellLanguage`, as a file might consist of cells when it is a notebook, but the file language is not a cell language ### Linked issues Breaks up #3586 ### Functionality - [x] rewrote code linting related resources ### Tests - [x] Added tests for `LocalFile`
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 5, 2025
…t from the context (#3638) <!-- REMOVE IRRELEVANT COMMENTS BEFORE CREATING A PULL REQUEST --> ## Changes Make session state public on `LinterContext` and access it from the context instead of passing it along. ### Linked issues Breaks up #3586 ### Functionality - [x] rewrote code linting related resources ### Tests - [x] Reusing existing tests
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 5, 2025
## Changes Improve file handling by `FileLoader`: - Verify the `FileLoader` resolves files using the `PathLookup` - Handle file opening errors, like `PermissionError`, `FileNotFoundError` - Handle file with BOM encodings - Handle empty files ### Linked issues Breaks up #3586 ### Functionality - [x] rewrote code linting related resources ### Tests - [x] Added tests for `FileLoader`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Let
LocalFileLinterreuse theDependencyinstead falling back on its path as we introduce duplicate ways of loading files.In addition to that, the PR contains a clean up of:
source_code/linters/files.pytosource_code/files.pyto avoid circular importsMockPathLookupto verify which paths are resolvedcurrent_sessionpublic onLinterContextfor reuse instead of passing it next to theLinterContextFileLoaderLinked issues
Progresses #3514
Breaks up #3520
Functionality
databricks labs ucx migrate-local-codeTests