Skip to content
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

Definitions #43

Merged
merged 13 commits into from
Nov 21, 2022
Merged

Definitions #43

merged 13 commits into from
Nov 21, 2022

Conversation

ivojawer
Copy link
Contributor

@ivojawer ivojawer commented Nov 13, 2022

El famoso ctrl+click llega a tu lenguaje de objetos favorito!!

Incluye y se limita a las definiciones de los siguientes nodos:

  • Self
  • New
  • Reference
  • Send
Screencast.2022-11-13.15.13.33.mp4

Known issues:

  • Cuando intenta navegar a una definicion de algo que esta dentro de donde hubieras navegado (e.g. self intenta navegar al module que contiene el metodo que llama al self) ignora la navegacion e intenta obtener las referencias. Definicion se cancela e invoca referencias #44

Copy link
Contributor

@fdodino fdodino left a comment

Choose a reason for hiding this comment

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

una hermosura! solo dejo unos comentarios porque me parece que hay responsabilidades que podría cubrir wollok-ts (se podría mergear este PR, revisar luego wollok-ts para ver si hace falta un refactor y crear un nuevo PR donde se delegue a wollok-ts cosas como allMethodDefinitions o superMethodDefinition).

import { Environment, is, Method, Module, Node, Reference, Send, Super } from 'wollok-ts'

export const getNodeDefinition = (environment: Environment) => (node: Node): Node[] => {
switch(node.kind){
Copy link
Contributor

Choose a reason for hiding this comment

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

Recuerdo que @nscarcella tenía un switch más cheto para wollok-ts, por ahí podríamos usarlo (ahora ya no recuerdo cómo se llamaba, creía que match...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cierto! Alguna vez lo habia visto pero no recordaba 💕

Comment on lines 47 to 61
function allMethodDefinitions(environment: Environment, send: Send): Method[] {
const arity = send.args.length
const name = send.message
const methods: Method[] = []
environment.forEach(n => {
if(
n.kind === 'Method' &&
n.name === name &&
n.parameters.length === arity
) {
methods.push(n)
}
})
return methods
}
Copy link
Contributor

Choose a reason for hiding this comment

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

de nuevo, acá lo cito a @nscarcella , estoy seguro de que hay algunas definiciones en wollok-ts que te deberían servir para resolver ésto, y si no me parece que esta responsabilidad (al igual que superMethodDefinition) debería estar ahí...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No los pude encontrar 😿 , lo unico parecido es en runtime que se hace una logica muy parecida al super method definition: https://github.com/uqbar-project/wollok-ts/blob/bd147e6ecbb859c06bc2f4e5a1b65cd5b5406069/src/interpreter/runtimeModel.ts#L508-L509

@@ -1,10 +1,12 @@
import { CompletionItem, CompletionItemKind, Connection, Diagnostic, DiagnosticSeverity, InsertTextFormat, Location, Position, TextDocumentPositionParams } from 'vscode-languageserver'
import path from 'path'
Copy link
Contributor

Choose a reason for hiding this comment

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

acá salta una validación de linter que corre github actions, por ahí podríamos tener un pre-commit que corra run lint:fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sii 100% ahora lo armo en otro PR que seguro saltan un monton de files

@@ -0,0 +1,35 @@
import { expect } from 'expect'
Copy link
Contributor

Choose a reason for hiding this comment

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

maravilloso! 👏🏼

Comment on lines +41 to +44
environment.forEach(node => {
if (node.sourceFileName() && include(node, textDocumentPosition)) result.push(node)
})
return result
Copy link
Contributor

Choose a reason for hiding this comment

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

es el segundo forEach + push que veo, siento que environment quiere un filter a gritos...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Voy a dejar esto por aqui

uqbar-project/wollok-ts#141

@ivojawer ivojawer merged commit 859aceb into master Nov 21, 2022
@ivojawer ivojawer deleted the definition branch November 21, 2022 01:31
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