-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: add custom editor / visualizer to vscode extension #1243
base: main
Are you sure you want to change the base?
Conversation
… statement node expandable
🦙 MegaLinter status:
|
Descriptor | Linter | Files | Fixed | Errors | Elapsed time |
---|---|---|---|---|---|
✅ CSS | stylelint | 3 | 0 | 0 | 1.54s |
✅ JAVASCRIPT | eslint | 4 | 0 | 0 | 2.96s |
prettier | 4 | 0 | 1 | 0.61s | |
✅ JSON | jsonlint | 10 | 0 | 0.2s | |
✅ JSON | npm-package-json-lint | yes | no | 0.85s | |
prettier | 79 | 0 | 1 | 1.48s | |
✅ JSON | v8r | 10 | 0 | 16.3s | |
✅ MARKDOWN | markdown-link-check | 1 | 0 | 0.72s | |
✅ REPOSITORY | git_diff | yes | no | 0.26s | |
✅ TSX | eslint | 66 | 0 | 0 | 13.51s |
✅ TYPESCRIPT | eslint | 41 | 0 | 0 | 10.1s |
prettier | 41 | 0 | 1 | 0.53s |
See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true
in mega-linter.yml to validate all sources, not only the diff
@GideonKoenig Please start by fixing the failing checks (builds, lint, PR title). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First batch of comments about the stub changes.
packages/safe-ds-lang/src/resources/builtins/safeds/lang/ideIntegration.sdsstub
Outdated
Show resolved
Hide resolved
packages/safe-ds-lang/src/resources/builtins/safeds/lang/ideIntegration.sdsstub
Outdated
Show resolved
Hide resolved
packages/safe-ds-lang/src/resources/builtins/safeds/lang/ideIntegration.sdsstub
Show resolved
Hide resolved
packages/safe-ds-lang/src/resources/builtins/safeds/data/tabular/plotting/TablePlotter.sdsstub
Outdated
Show resolved
Hide resolved
### Summary of Changes Modifies the language stubs in preparation for the Custom Editor/Visualization PR #1243. Defines/Adjusts the DataScienceCategories and assigns them to a sample amount of functions and classes from the Safe-Ds language. --------- Co-authored-by: Lars Reimann <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lars-reimann
Apparently I missed this change in the PR before. Should I extract the two remaining category assignments into a separate branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would make sense, so these changes quickly end up on main
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be included in the repo. Code should look consistent, regardless of the sub-project.
Locally, you can of course reformat the (TypeScript) code, however you want.
{ | ||
files: ['packages/safe-ds-editor/src/**'], | ||
rules: { | ||
'import/no-default-export': 'off', | ||
'no-console': 'off', | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a reason to disable these rules completely for this folder.
- Default exports are rarely needed, and using named exports throughout leads to more consistent code and a more consistent API. Moreover, we provide names for the exported elements, which already indicate their purpose. Default exports are unnamed, so users must always refer to documentation or the module that exports them.
- Console statements are very easy to forget in the code. You are better off disabling the error for the few lines where you need them by adding a comment directly above the affected lines (https://eslint.org/docs/latest/use/configure/rules#disabling-rules).
@@ -0,0 +1,5 @@ | |||
{ | |||
"files.associations": { | |||
"*.css": "tailwindcss" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this require any extension to be installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment about updating prettierrc
. Should not be included in the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd greatly prefer this to be included in ESBuild script in the safe-ds-vscode
project (packages\safe-ds-vscode\esbuild.mjs
). It's always better if a project manages its dependencies rather than its dependents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If default exports are unavoidable here, add a comment to disable the error for the next line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next step would be to extract the custom-editor
service into its own PR. It also requires unit tests. Finally, "custom-editor" is a very general name, maybe something like "graphical-editor" is better (or currently "graphical-view", though then we'd have to rename it eventually). I'm open for suggestions here.
static callList: Call[]; | ||
static genericExpressionList: GenericExpression[]; | ||
static edgeList: Edge[]; | ||
static safeDsServices: SafeDsServices; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything that needs services should be made a service, so everything gets wired up properly. Here, Utils
is in an invalid state before initialize
is called.
Likewise, it should not have static state/methods but instance state/methods, since multiple instances of SafeDsServices
might be in use at the same time, e.g. when running tests. Using static state here, can quickly lead to subtle bugs.
export const zip = <A, B>(arrayA: A[], arrayB: B[]): [A, B][] => { | ||
const minLength = Math.min(arrayA.length, arrayB.length); | ||
const result: [A, B][] = []; | ||
|
||
for (let i = 0; i < minLength; i++) { | ||
result.push([arrayA[i]!, arrayB[i]!]); | ||
} | ||
|
||
return result; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be a useful addition to packages\safe-ds-lang\src\helpers\collections.ts
.
## [0.22.0](v0.21.1...v0.22.0) (2024-11-24) ### Features * categorize API elements ([#1263](#1263)) ([d0d971e](d0d971e)), closes [#1243](#1243) * disable inlay hints for assignee types by default ([#1260](#1260)) ([a651ade](a651ade)) * improve error messages from linker ([#1272](#1272)) ([eddd868](eddd868)), closes [#1268](#1268) * output statement ([#1262](#1262)) ([011ba31](011ba31)), closes [#1259](#1259) * remove schema concept from grammar ([#1273](#1273)) ([1a3bf80](1a3bf80)), closes [#1133](#1133)
Giant Pull Request - All in one - I'm sorry