Add initial implementation of framework mode#2535
Conversation
koesie10
force-pushed
the
koesie10/framework-mode
branch
from
2ccee67 to
4a94685
Compare
koesie10
force-pushed
the
koesie10/framework-mode
branch
from
4a94685 to
8c36e57
Compare
starcke
left a comment
starcke
left a comment
There was a problem hiding this comment.
I love that we already have a rough version of framework mode! I do think that it is a bit more rough than the normal mode, (kinda like the poc we did). I think this can go in now and then next week we can discuss what next steps should be for this mode.
|
|
||
| // In application mode, we need the database of a specific library to generate | ||
| // the modeled methods. In framework mode, we'll use the current database. | ||
| if (this.mode === Mode.Application) { |
There was a problem hiding this comment.
There are quite a lot of mode checks. It is probably ok, but if we start to get even more we might want to consider some kind of mode abstraction, so that we dont have to do an if in all the logic.
There was a problem hiding this comment.
Definitely. I think for now with just two modes this works fine, but this will break down if we have more modes.
| return result; | ||
| } | ||
|
|
||
| export function createDataExtensionYamlsForFrameworkMode( |
There was a problem hiding this comment.
Something to think about, is that eventually it would be amazing if modelling library X while in application mode for A, would yield the same file as modelling library X in framework mode.
There was a problem hiding this comment.
Yes, I think this is something to think about. For now, I don't think we can really detect this reliably because we're depending on two different inputs (database vs JAR name).
| {externalApiUsage.usages.length} | ||
| </UsagesButton> | ||
| </VSCodeDataGridCell> | ||
| {mode === Mode.Application && ( |
There was a problem hiding this comment.
Hmm... these mode comparisons do go in many places.
| c.(Constructor).isParameterless() | ||
| } | ||
|
|
||
| class PublicMethod extends Callable { |
There was a problem hiding this comment.
For my understanding: this is basically a copy of ExternalAPI with the constructor changed? I cannot completely parse whether that is exactly what we want, but I think at a minimum it is close.
I think moving to the CodeML queries is something we should do, otherwise we probably want to make some changes (e.g. sharing the code for all those methods).
There was a problem hiding this comment.
Yes, this basically changes not this.fromSource() to this.isPublic() in the constructor. The rest of the methods are very similar. We can work on reducing the duplication in a follow-up.
extensions/ql-vscode/src/data-extensions-editor/queries/java.ts
Outdated
Show resolved
Hide resolved
| usage = aUsage(api) | ||
| select usage, apiName, supported.toString(), "supported", api.getFile().getBaseName(), "library" | ||
| `, | ||
| frameworkModeQuery: `/** |
There was a problem hiding this comment.
Same comments as the java method below.
2 participants
This adds an initial rough implementation of framework mode for the data extensions editor. Framework mode will do the following:
Short demo:
Screen.Recording.2023-06-21.at.16.53.06.mov
Checklist
ready-for-doc-reviewlabel there.