Skip to content

Move contextual and ast-viewer files into language-support#2368

Merged
norascheuch merged 7 commits intomainfrom
nora/move-files-misc1
Apr 26, 2023
Merged

Move contextual and ast-viewer files into language-support#2368
norascheuch merged 7 commits intomainfrom
nora/move-files-misc1

Conversation

@norascheuch
Copy link
Copy Markdown
Contributor

@norascheuch norascheuch commented Apr 24, 2023

This PR is part of an endeavour to order files in the codebase according to the domain model. Here the folder src/contextual is moved into language-support.

  • some methods and classes inside the contextual folder were using default to export. These defaults were removed and an index.ts introduced
  • some tests were moved to a new language-support/contextual- and language-support-ast-viewer-folder
  • for 2 ast-viewer related data files a new folder and adjusted paths had to be added

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@norascheuch norascheuch requested a review from a team as a code owner April 24, 2023 12:03
@norascheuch norascheuch force-pushed the nora/move-files-misc1 branch from f5051fd to d77e7b3 Compare April 24, 2023 12:22
@norascheuch norascheuch changed the title Move contextual query files Move contextual and ast-viewer files into language-support Apr 24, 2023
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Generally looking good, just some minor comments/questions.

@@ -1,3 +1,4 @@
export * from "./contextual";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you mean to export everything from contextual to the outside world? I would have thought that some of it is only internal to language-support (e.g. the file-range-from-uri stuff).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Discussed synchronously, fixed

Comment thread extensions/ql-vscode/src/language-support/ast-viewer/ast-cfg-commands.ts Outdated
Comment thread extensions/ql-vscode/src/language-support/contextual/location-finder.ts Outdated
Comment thread extensions/ql-vscode/src/language-support/contextual/index.ts Outdated
@norascheuch norascheuch force-pushed the nora/move-files-misc1 branch from d77e7b3 to 38ee043 Compare April 25, 2023 13:46
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Looks good, just some potential improvements.

import type { CommandManager } from "../packages/commands";
import type { Uri, Range, TextDocumentShowOptions } from "vscode";
import type { AstItem } from "../astViewer";
import type { AstItem } from "../language-support/ast-viewer/ast-viewer";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this missing from index.ts?

Comment thread extensions/ql-vscode/src/extension.ts Outdated
Comment thread extensions/ql-vscode/src/extension.ts
@norascheuch norascheuch force-pushed the nora/move-files-misc1 branch from 38ee043 to b6b3d3b Compare April 26, 2023 09:56
@norascheuch norascheuch merged commit 77fbaea into main Apr 26, 2023
@norascheuch norascheuch deleted the nora/move-files-misc1 branch April 26, 2023 12:15
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