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

User natives #326

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

User natives #326

wants to merge 10 commits into from

Conversation

lgassman
Copy link

Permite construir un Intérprete que reciba funciones nativas escritas por el usuario

El principal cambio es que en lugar de importar las natives de WRE, se debe importar la función natives(), la cual se invoca conas las natives de usuario (o vacío si no hay) y devuelve un merge de éstas con las nativas del WRE.

Agrega un deepEquals en los assertions para verificar que el merge se produzca como corresponde

Copy link

codecov bot commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (d595ac1) to head (d2ba7f6).

Files with missing lines Patch % Lines
src/interpreter/interpreter.ts 66.66% 2 Missing ⚠️
src/wre/natives.ts 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #326      +/-   ##
==========================================
- Coverage   89.78%   89.73%   -0.05%     
==========================================
  Files          28       29       +1     
  Lines        3191     3205      +14     
  Branches      581      583       +2     
==========================================
+ Hits         2865     2876      +11     
- Misses        171      173       +2     
- Partials      155      156       +1     

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

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.

👍🏼

"skipFiles": [
"<node_internals>/**"
],
"type": "node-terminal"
Copy link
Contributor

Choose a reason for hiding this comment

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

qué onda este cambio?

Copy link
Author

Choose a reason for hiding this comment

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

Quizás debería dejarlo como estaba, no estoy seguro como estaba antes, pero me resultó mucho más cómodo para integrar con el debugger correr con node-terminal y agregar un run test:file

tsconfig.json Outdated
@@ -12,16 +13,20 @@
"moduleResolution": "node",
"resolveJsonModule": true,
"esModuleInterop": true,
"experimentalDecorators": true
"experimentalDecorators": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

no puedo creer que no tuviéramos este flag antes

package.json Show resolved Hide resolved
test/natives.test.ts Outdated Show resolved Hide resolved
test/natives.test.ts Outdated Show resolved Hide resolved
@lgassman lgassman requested a review from ivojawer December 30, 2024 01:13
lgassman and others added 2 commits December 29, 2024 22:17
Co-authored-by: Fernando Dodino <[email protected]>
Co-authored-by: Fernando Dodino <[email protected]>
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.

Al tokeeeee 🚀 🌴 🗻 🌊

compareMaps(obj1, obj2) ||
compareSets(obj1, obj2)

Assertion.addMethod('deepEquals', function (this: Chai.AssertionStatic, expected: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hace falta este helper? No se puede usar value.should.deep.eq(expected)?

@PalumboN
Copy link
Contributor

PalumboN commented Jan 2, 2025

Otra cosa, esto estaría bueno que quede documentado en algún lado

Copy link
Contributor

@ivojawer ivojawer left a comment

Choose a reason for hiding this comment

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

Grosooo

src/interpreter/interpreter.ts Outdated Show resolved Hide resolved
@PalumboN
Copy link
Contributor

PalumboN commented Feb 2, 2025

@lgassman @ivojawer @fdodino ahí le terminé de dar una pasada al PR:

  • Mergié master
  • Tuve que sacar la opción de composite del tsconfig. Me explotaba el IDE. Pero igual así funciona (lo probé con la branch de la CLI y pude correr tus ejemplos @lgassman).
  • Y resolví el comentario de Ivo

También hice que el comando wtest lea natives del proyecto (no tiene la lógica de seleccionar otra carpeta en el package.json, me dio paja). Pero así es más fácil de probar esto sin necesidad de la CLI.

Si querés confirmar que te anda todo en tu máquina mejor (tal vez funciona distinto en mac, linux, windows?). Y mergeamos! :)

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