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

Rename #127

Merged
merged 12 commits into from
Jan 7, 2024
Merged

Rename #127

merged 12 commits into from
Jan 7, 2024

Conversation

ivojawer
Copy link
Contributor

Esta seria una primer version muy limitada de renamings

Los nodos que se pueden renombrar son

  • Fields (que no sean properties)
  • Parameters
  • Variables
  • Y las refencias a ellos

Cualquier otro nodo sera rechazado

Screen.Recording.2023-12-16.at.20.52.54.mov

Hay un known issue cuando la sentencia es un assignment abreviado:

energia += comida.calorias()

Porque en esa sentencia hay dos "energia" con el mismo sourcemap, una del assignment y otra de la referencia, en mi opinion la referencia deberia ser sintetica y en ese caso se ignoraria. Que opinan?

PD: Arregle un problema con el debugger donde los archivos en subcarpetas no eran debuggeables

Copy link

codecov bot commented Dec 17, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (ba30dab) 82.00% compared to head (91c7cdf) 80.97%.

Files Patch % Lines
server/src/utils/text-documents.ts 50.00% 4 Missing ⚠️
server/src/utils/vm/wollok.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
- Coverage   82.00%   80.97%   -1.04%     
==========================================
  Files           8        8              
  Lines         239      247       +8     
  Branches       69       70       +1     
==========================================
+ Hits          196      200       +4     
- Misses         43       47       +4     
Flag Coverage Δ
lsp-ide-server 80.97% <50.00%> (-1.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ivojawer ivojawer marked this pull request as draft December 17, 2023 00:05
Copy link
Contributor

@PalumboN PalumboN left a comment

Choose a reason for hiding this comment

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

Siempre un paso adelante Ivo!! 📈 🏃 💯

Faltaría algún test , no?

Y el problema de las asignaciones con += -= está en
https://github.com/uqbar-project/wollok-ts/blob/master/src/parser.ts#L353
habría que marcar el send y el receiver como sintéticos.

Comment on lines +14 to +16
changes: renamedNode.is(Reference) ?
groupByURI(renameNode(renamedNode.target!, params.newName, environment, documents)) :
groupByURI(renameNode(renamedNode, params.newName, environment, documents)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
changes: renamedNode.is(Reference) ?
groupByURI(renameNode(renamedNode.target!, params.newName, environment, documents)) :
groupByURI(renameNode(renamedNode, params.newName, environment, documents)),
changes: groupByURI(renameNode(renamedNode.is(Reference) ? renamedNode.target! : renamedNode, params.newName, environment, documents))

Y qué onda si no tiene target? (Una referencia rota)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saque todas las validaciones porque en requestIsRenamable ya se valida y siempre corre antes

const renamedNode = cursorNode(environment, params.position, params.textDocument)
if(!renamedNode) return null
if( renamedNode.is(Reference) && renamedNode.target && isRenamable(renamedNode.target) || isRenamable(renamedNode)) {
// ToDo: switch back to defaultBehavior when https://github.com/microsoft/vscode/issues/198423 is released
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

server/src/utils/vm/wollok.ts Outdated Show resolved Hide resolved
@ivojawer ivojawer marked this pull request as ready for review January 4, 2024 15:40
Copy link
Contributor

@PalumboN PalumboN left a comment

Choose a reason for hiding this comment

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

Vamooooooooo

client/src/test/definition.test.ts Outdated Show resolved Hide resolved
Comment on lines +18 to +20
const edition = new WorkspaceEdit()
edition.replace(docUri, new Range(new Position(3, 15), new Position(3, 21)), 'unaComida')
edition.replace(docUri, new Range(new Position(4, 24), new Position(4, 30)), 'unaComida')
Copy link
Contributor

Choose a reason for hiding this comment

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

Chetoooooo 📈

Esto podría estar afuera y reutilizarse en el otro test

@ivojawer ivojawer merged commit ebeeaa1 into master Jan 7, 2024
4 of 6 checks passed
@ivojawer ivojawer deleted the rename branch January 7, 2024 16:52
@fdodino
Copy link
Contributor

fdodino commented Jan 11, 2024

Qué groso @ivojawer ! 🎉

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.

4 participants