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

[EPIC] Commands enhancements #115

Merged
merged 51 commits into from
Nov 21, 2023
Merged

[EPIC] Commands enhancements #115

merged 51 commits into from
Nov 21, 2023

Conversation

fdodino
Copy link
Contributor

@fdodino fdodino commented Nov 5, 2023

Cosas sueltas que quiero arreglar aprovechando la próxima versión de wollok-ts-cli

Al REPL hay que

  • refactorizarlo como en el archivo test.ts, que haya funciones más chicas para que sea más fácil testearlo
  • también hay que extraer la función que agrega el diagrama para que sea fácil el comando :d después (esto lo desacoplé utilizando una interfaz que permita definir el evento onReload indicando si hay que activar el diagrama)
  • mejorar la opción :d que en realidad solo recarga el diagrama. Ahora si no estaba levantado el diagrama lo levanta en el puerto que vos le pasaste (por defecto es el 3000, si querés levantarlo en otro puerto tenés que indicarlo antes)
  • agregarle testeo unitario, validar el comportamiento en casos borde
  • quería ponerle además un ping cuando levanta que confirme que no nos va a tirar un 404 la página http://localhost:3000, tengo que investigar cómo se hace eso
  • Meter el comando reload and re-run Codear el comando "reload and rerun" del cli #20

Y de paso, mejorar la cobertura de tests.

  • agregar tests de utils
  • incorporar tests para init que hoy no está cubierto (y el % no los considera)
  • definir tests para el testeo unitario
  • agregar sinon para mockear (no es la biblioteca que más me gusta pero se integra bien con mocha)

Para otro PR dejamos:

Copy link

codecov bot commented Nov 5, 2023

Codecov Report

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

Comparison is base (324ce23) 73.13% compared to head (d49aa05) 91.28%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/commands/repl.ts 68.00% 24 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #115       +/-   ##
===========================================
+ Coverage   73.13%   91.28%   +18.14%     
===========================================
  Files           3        5        +2     
  Lines         309      459      +150     
  Branches       82      107       +25     
===========================================
+ Hits          226      419      +193     
+ Misses         83       40       -43     

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

@fdodino fdodino changed the base branch from master to update-ci November 5, 2023 12:09
Base automatically changed from update-ci to master November 7, 2023 22:50
@fdodino fdodino changed the title Commands enhancements [EPIC] Commands enhancements Nov 8, 2023
@fdodino fdodino marked this pull request as draft November 13, 2023 23:46
@fdodino fdodino marked this pull request as ready for review November 18, 2023 21:48
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.

GRANDIOSO FER!! 🥳 🎈

Alto laburo!! Qué prolojidad y cuanto amor tienen esos tests 📈

Dejé algunos comments y sugerencias como de costumbre (:

src/commands/init.ts Show resolved Hide resolved
src/commands/repl.ts Outdated Show resolved Hide resolved
src/commands/repl.ts Show resolved Hide resolved
src/commands/repl.ts Outdated Show resolved Hide resolved
src/commands/test.ts Outdated Show resolved Hide resolved
src/index.ts Outdated
@@ -40,6 +43,7 @@ program.command('repl')
.option('-p, --project [filter]', 'path to project', process.cwd())
.option('--skipValidations', 'skip code validation', false)
.option('--darkMode', 'dark mode', false)
.option('--noDiagram', 'avoid starting the server for the dynamic diagram', false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Me costó estos parámetros en negativo. Por qué no tenerlos en positivo y con true como default?

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 es que no considere que no tenés razón... 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm... pero no me convence tener que decirle --diagram false para que no ejecute. El skipValidations ya estaba, no te convence si le ponemos skipDiagram? Yo se que es medio feucho, pero te propongo que si en un par de meses nos jode hagamos el refactor...

@@ -48,6 +52,11 @@ program.command('repl')
program.command('init')
.description('Create a new Wollok project')
.option('-p, --project [filter]', 'path to project', process.cwd())
.option('-n, --name [name]', 'name of the example', undefined)
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
.option('-n, --name [name]', 'name of the example', undefined)
.option('-n, --name [name]', 'name of the project', undefined)

O a qué te referís con example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Es que el project tiene un path, es raro que le pongas otro nombre...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

igual tenés razón, qué onda "name of the project example file"

Comment on lines +65 to +67
else if(problems.some(_ => _.level === 'error')) {
throw new Error('Aborting run due to validation errors!')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Esto queremos tenerlo en esta función?

TENGO una propuesta:

  • Que siempre se "validen" los programas -> logguear los problemas que tenga
  • Que el skipValidations solamente (no) aborte en caso de errores

Copy link
Contributor Author

Choose a reason for hiding this comment

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

me gusta la idea pero es algo que ya estaba, y preferiría: 1. charlarlo con más gente, 2. no meterlo dentro de este PR porque eso implica volverme a sentar a revisar los tests, y hacer tests manuales...

src/utils.ts Outdated Show resolved Hide resolved
test/test.test.ts Outdated Show resolved Hide resolved
@fdodino fdodino merged commit ccb8f73 into master Nov 21, 2023
3 checks passed
@fdodino fdodino deleted the commands-enhancements branch November 21, 2023 21:16
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