Init :: Sanitize name and paths #193

nmigueles · 2024-09-30T02:06:43Z

~~Fixed path assertions that were wrong, tried to make a failing test and was not failing 💀~~ Ahora en Fix de pathAssertion #205
Added lodash as a dependency (free to discuss)
Normalized package.json to be kebab case (Fixes Warning cuando el init genera un nombre de package #155)
Normalized project names to be sanitized and camel cased (Fixes opción "name" del cli no debería dejar usar caracteres especiales #188)
Now imports and filenames used the normalized project name

nmigueles · 2024-10-02T23:48:58Z

#188 (comment)
Gracias por el catch @asanzo, ahi probe con un -n '' para forzar el caso del nombre vacío y obvio explotaba por los aires.

Ya esta fixeado el caso

PalumboN

Groso @nmigueles !!! 💯 🐫

Ahí dejé los comentarios que te había contado y me habían quedado colgado hace un montón jajaj

Entiendo que parte de esto lo charlaste con @asanzo , me gustaría que saber qué piensa de mis comentarios 🤔

PalumboN · 2024-10-08T05:29:39Z

package.json

@@ -55,6 +55,7 @@
    "cytoscape": "^3.15.2",
    "express": "^4.18.1",
    "globby": "^11.0.4",
+    "lodash": "^4.17.21",


Uhhh polémico 😆

Esto se usa para las funciones camelCase y kebabCase.

Quizá podemos encontrar alguna implementación por ahí y evitar la dependencia.

encontré estas bibliotecas

https://www.npmjs.com/package/kebab-case

https://www.npmjs.com/package/camelcase

@nmigueles

PalumboN · 2024-10-08T05:34:18Z

test/assertions.ts

+  Assertion.addMethod('pathExists', function () {
+    const path = this._obj


PalumboN · 2024-10-08T05:37:11Z

src/commands/init.ts

-  const exampleName = name ?? 'example'
-  logger.info(`Creating definition file ${exampleName}.${WOLLOK_FILE_EXTENSION}`)
-  writeFileSync(join(project, `${exampleName}.${WOLLOK_FILE_EXTENSION}`), wlkDefinition)
+  const exampleName = name && name.length > 0 ? name : 'example'


PalumboN · 2024-10-08T05:42:16Z

src/commands/init.ts

-  logger.info(`Creating definition file ${exampleName}.${WOLLOK_FILE_EXTENSION}`)
-  writeFileSync(join(project, `${exampleName}.${WOLLOK_FILE_EXTENSION}`), wlkDefinition)
+  const exampleName = name && name.length > 0 ? name : 'example'
+  const exampleFilename = sanitizeName(exampleName)


Mmmm tengo dudas sobre esto...

A mí me gusta que los programas entiendan lo que quiero hacer y, o bien lo hagan, o bien me tiren un error.
No sé si este comportamiento entra en alguna de esas.

Si me la estoy mandando en el nombre que elegí, tal vez sea mejor tirar un error.

Bueno, bien. Esta es la discusión interesante que quería tener.

Recordando lo frustrante que es elegir una contraseña en un banco y que te la vaya rechazando hasta que le pegás a cuántas mayúsculas y números quiere que tengas, y recordando la premisa que usamos en otros lugares de desburocratizar la UX (como cuando establecimos que el creador de desafíos de Pilas Bloques debía siempre armar desafíos válidos, en lugar de tirar popups de "te falta el personaje" o "no se puede poner un personaje encima de...." etc.), se me ocurrió la propuesta de "dejame pasar todo, corregí lo que quieras, sólo dame mi proyecto Wollok"

Pero no me vuelvo loco si quieren mostrar un error del estilo "recordá que los nombres de proyecto deben empezar con minúscula, no incluir caracteres raros. Pueden tener letras, números y guiones bajos pero no espacios o eñes". y listo.

sí, yo prefiero no tomar decisiones por el usuario. Si elegís un nombre incorrecto, que falle es lo más sano (salvo en el package.json que no me interesa mucho que tenga un nombre porque esa validación no es nuestra ni tampoco la queremos)

src/commands/init.ts

PalumboN · 2024-10-21T20:31:36Z

src/utils.ts

+ * Replaces every symbol not allowed with a '_'.
+ */
+export const sanitizeName = (name: string): string => camelCase(name[0].replace(/([^a-z_])/g, '_$1') + name.slice(1).replace(/[^a-zA-z1-9_-]/g, '_'))


Creo que acá se debería re-usar la misma lógica que la validación (habría que hacer un refactor de la función):
https://github.com/uqbar-project/wollok-ts/blob/6ad63ac1f8ad7b069eb61204ab897f8cd49efa0c/src/validator/index.ts#L458-L461

De todas formas, yo creo que el problema del issue #188 es que los archivos se están creando con el mismo nombre que el proyecto.... yo no sé si quiero eso.

Yo voto por elegir el nombre del proyecto (carpeta base) y que los archivos siempre se llamen example.wlk, etc.

Ok, ahí me di cuenta que el init crea los archivos pero no la carpeta base... En ese caso esperaría que:

si no le digo nada, tenga el cuenta el nombre del directorio en donde estoy

si le doy un nombre, me cree una carpeta

No sé, tiro comments para debatir 🍡

El problema del issue 118 es que se están usando caracteres en lugares donde no andan, como marco en opción "name" del cli no debería dejar usar caracteres especiales #188 (comment)

Para mí el name debería ser la cosa más safe del mundo. Acá en el código que mencionás, Rasta lindo, si entiendo bien esa regex, incluye puntos y barras, que no los quiero, como menciono en el comentario anterior.

Sobre los defaults:

No me gusta la de tomar el nombre del directorio. Es raro el comportamiento, no siempre creo una carpeta especialmente. Es sucio, lo sé, pero haría que se llame de formas raras.

Si vamos a crear una carpeta, el nombre debería ser obligatorio para mí.

De todas formas, opino que no quiero crear una carpeta (el git init no lo hace, el npm init tampoco...)

Si no creamos la carpeta, podría tener "example" como nombre de proyecto por defecto y ya, o lo que quieran.

No tengo una opinión muy fuerte respecto de tener el nombre del proyecto en los nombres de los archivos. Siento que me gusta que los proyectos tengan nombres de archivos distintos, y por otro lado siento que me gusta la simplicidad de que se llamen todos igual.

No me gusta la de tomar el nombre del directorio. Es raro el comportamiento, no siempre creo una carpeta especialmente. Es sucio, lo sé, pero haría que se llame de formas raras.

+1 y comenté sobre el cambio de exampleFileName. Yo separaría por un lado el proyecto del nombre del example.

De todas formas, opino que no quiero crear una carpeta

stack test sí lo hace, CRA (create-react-app) sí lo hace, svelte-kit sí lo hace. Esta cuestión es debatible, lo que por ahí tiene a favor es que obliga a una persona a crear la carpeta previamente y eso no te va a dar el error de hacer code . en la carpeta raíz como pasa con el 20% de los pibes que reportan errores en Haskell.

Por otra parte, vas a tener el 20% de pibes que van a quejarse cuando creen su segundo proyecto porque van a olvidarse de crear la carpeta y van a tener todo en un gran directorio.

PalumboN · 2024-10-21T20:36:00Z

test/init.test.ts

+  it('should normalize package.json name when creating with custom name', async () => {
+    init(undefined, { ...baseOptions, name: 'Pepita Game' })
+
+    expect(join(project, 'pepitaGame.wlk')).to.pathExists()
+    expect(join(project, 'testPepitaGame.wtest')).to.pathExists()
+    expect(join(project, 'package.json')).to.pathExists()
+    expect(join(project, GITHUB_FOLDER, 'ci.yml')).to.pathExists()
+    expect(join(project, 'README.md')).to.pathExists()
+    expect(join(project, '.gitignore')).to.pathExists()
+
+    // Assert content of package.json
+    const packageJson = readFileSync(join(project, 'package.json'), 'utf8')
+    const { name } = JSON.parse(packageJson)
+    expect(name).to.be.equal('pepita-game')
+  })


Sé que dije de no tener este comportamiento, pero debo admitir que este test me cabe.

seh, es hermoso.

jajaja, banco!

nmigueles · 2024-12-02T01:51:13Z

Buenas, retomando este PR, para mi la cuestion se divide en 3 partes atacables: (que podrian ser prs por separado para simplificar las discuciones)

Arreglar el assertion sobre los paths
Normalizar el nombre del proyecto a snake case para al package.json (aca no tenemos otra que mandarle algo valido y para mi lo mejor seria hacerle un kebabCase y olvidarse, obvio haciendo una implementación propia para evitar tener lodash como dependencia)
Normalizar / Limitar / X el nombre de los archivos, que mi take aca es que el name del proyecto no tenga injerencia en los nombres de los archivos, es decir, que siempre se llamen de la misma manera, esto puede ser util para generar cierto acostumbramiento en los alumnos y de sobre todo simplificar los tutoriales / docus.

fdodino

bueno @nmigueles ahí dejé algunos comentarios sobre este PR

fdodino · 2024-12-03T00:25:20Z

src/commands/init.ts


  if (!noTest) {
-    const testFile = `test${capitalizeFirstLetter(exampleName)}.${TEST_FILE_EXTENSION}`
+    const testFile = `test${capitalizeFirstLetter(exampleFilename)}.${TEST_FILE_EXTENSION}`


ojo que deberíamos dejar exampleName y en todo caso validarlo/sanitizarlo

fdodino · 2024-12-03T00:25:36Z

src/commands/init.ts

  }

  if (game) {
-    const gameFile = `main${capitalizeFirstLetter(exampleName)}.${PROGRAM_FILE_EXTENSION}`
+    const gameFile = `main${capitalizeFirstLetter(exampleFilename)}.${PROGRAM_FILE_EXTENSION}`


lo mismo acá: es exampleName

fdodino · 2024-12-03T00:25:48Z

src/commands/init.ts

+  const exampleName = name && name.length > 0 ? name : 'example'
+  const exampleFilename = sanitizeName(exampleName)
+
+  const wollokDefinitionFile = `${exampleFilename}.${WOLLOK_FILE_EXTENSION}`


ver comentario más abajo exampleName

fdodino · 2024-12-03T00:30:56Z

src/utils.ts

+ * Replaces every symbol not allowed with a '_'.
+ */
+export const sanitizeName = (name: string): string => camelCase(name[0].replace(/([^a-z_])/g, '_$1') + name.slice(1).replace(/[^a-zA-z1-9_-]/g, '_'))


No me gusta la de tomar el nombre del directorio. Es raro el comportamiento, no siempre creo una carpeta especialmente. Es sucio, lo sé, pero haría que se llame de formas raras.

+1 y comenté sobre el cambio de exampleFileName. Yo separaría por un lado el proyecto del nombre del example.

De todas formas, opino que no quiero crear una carpeta

stack test sí lo hace, CRA (create-react-app) sí lo hace, svelte-kit sí lo hace. Esta cuestión es debatible, lo que por ahí tiene a favor es que obliga a una persona a crear la carpeta previamente y eso no te va a dar el error de hacer code . en la carpeta raíz como pasa con el 20% de los pibes que reportan errores en Haskell.

Por otra parte, vas a tener el 20% de pibes que van a quejarse cuando creen su segundo proyecto porque van a olvidarse de crear la carpeta y van a tener todo en un gran directorio.

fdodino · 2024-12-03T02:01:38Z

package.json

@@ -55,6 +55,7 @@
    "cytoscape": "^3.15.2",
    "express": "^4.18.1",
    "globby": "^11.0.4",
+    "lodash": "^4.17.21",


encontré estas bibliotecas

https://www.npmjs.com/package/kebab-case

https://www.npmjs.com/package/camelcase

@nmigueles

fdodino · 2024-12-03T02:28:22Z

test/assertions.ts

    const exists = existsSync(path)
+
+    const pathBasePath = path.split('/').slice(0, -1).join('/')


ésto funcionará para Windows?

Mismo que aca, lo voy a volar #193 (comment)

fdodino · 2024-12-03T02:29:18Z

test/assertions.ts

+    const pathBasePath = path.split('/').slice(0, -1).join('/')
+
+    // Improve error message if path does not exist
+    const files = !exists ? lstatSync(pathBasePath).isDirectory() ? readdirSync(pathBasePath) : [] : []


uh, áspero el : [] : []

See, todo esto lo pase a #205 y cuando retome esto cuando tenga un cacho lo vuelo de aca, lo simplifique para que funcione y no haga cosas raras

fdodino · 2024-12-03T02:31:43Z

test/init.test.ts

@@ -81,28 +82,80 @@ describe('testing init', () => {
      ...baseOptions,
      noCI: true,
      noTest: true,
+      game: true,


qué onda? por qué se agregó game: true?

Porque estaba en un limbo el test case, en el arrange seteaba un game: false, pero en el assert comprobaba archivos de game, asi que me parecio mas copado prenderlo

fdodino · 2024-12-03T02:34:15Z

test/init.test.ts

-    expect(join(project, '.gitignore')).to.pathExists
-    expect(join(project, 'README.md')).to.pathExists
+    expect(join(project, 'pepita.wlk')).to.pathExists()
+    expect(join(project, 'testPepita.wtest')).not.to.pathExists()


qué loco que el flag no lo usaba y en el test no lo corregí, bien ahí detectando eso

fdodino · 2024-12-03T02:35:02Z

test/init.test.ts

+  it('should normalize package.json name when creating with custom name', async () => {
+    init(undefined, { ...baseOptions, name: 'Pepita Game' })
+
+    expect(join(project, 'pepitaGame.wlk')).to.pathExists()
+    expect(join(project, 'testPepitaGame.wtest')).to.pathExists()
+    expect(join(project, 'package.json')).to.pathExists()
+    expect(join(project, GITHUB_FOLDER, 'ci.yml')).to.pathExists()
+    expect(join(project, 'README.md')).to.pathExists()
+    expect(join(project, '.gitignore')).to.pathExists()
+
+    // Assert content of package.json
+    const packageJson = readFileSync(join(project, 'package.json'), 'utf8')
+    const { name } = JSON.parse(packageJson)
+    expect(name).to.be.equal('pepita-game')
+  })


jajaja, banco!

PalumboN · 2025-01-05T18:32:38Z

Qué onda esto @nmigueles @asanzo @fdodino ? Dejamos escrito alguna forma de cerrar este PR?

fdodino · 2025-01-08T00:17:48Z

Qué onda esto @nmigueles @asanzo @fdodino ? Dejamos escrito alguna forma de cerrar este PR?

Dale, el jueves nos juntamos a cerrar una definición.

test/init.test.ts

fdodino · 2025-01-10T01:32:45Z

Bueno, acá nos juntamos el comité y unánimemente definimos

Crear o no crear la carpeta

init: debe crear una carpeta o asumir que hay que ubicarse primero en la carpeta?
hoy si ponés -p te crea un carpeta y si no ponés -p no te la crea.

Dejamos ese comportamiento.

Caracteres incorrectos en el proyecto o el nombre

qué pasa si elegís un nombre incorrecto (por un ejemplo un nombre con espacios, con caracteres inválidos tipo ?!@-?
La propuesta del PR era sanitizarlo.

No hay nada que pueda romper el proyecto, deberíamos dejarlo pasar. En el package.json había un warning, no pasa eso si tenés un profile con Wollok IDE y Wollok Highlighting. **Propuesta: ** usar la misma función que usa la validation (ver si está expuesta) para asegurarnos de que no tire errores linter.

Nombres de los archivos wlk, wtest

los nombres de los archivos por defecto: hoy podés elegir -p para el nombre del proyecto y -n para el nombre del ejemplo, si usás

wollok init -p proyectito -n pepita

te genera una carpeta proyectito que tiene adentro pepita.wlk y testPepita.wtest, por defecto si no te deja example.wlk y testExample.wtest.

Qué queremos: Mantener la propuesta

nmigueles added 3 commits September 29, 2024 22:56

fix test assertions

3ed28a6

fix sanitization in init components

48445e8

add @asanzo sanitization function with some tweaks

3bfe59f

nmigueles mentioned this pull request Sep 30, 2024

opción "name" del cli no debería dejar usar caracteres especiales #188

Open

add empty name case

52cbcfb

PalumboN requested review from fdodino, PalumboN and ivojawer October 8, 2024 05:12

PalumboN reviewed Oct 21, 2024

View reviewed changes

PalumboN mentioned this pull request Oct 21, 2024

El init debería sacar la versión de Wollok del package.json #198

Open

nmigueles mentioned this pull request Dec 2, 2024

Fix de pathAssertion #205

Merged

fdodino reviewed Dec 3, 2024

View reviewed changes

nmigueles marked this pull request as draft December 13, 2024 03:03

Merge branch 'master' into package-name-normalization

4a07d69

nmigueles commented Jan 9, 2025

View reviewed changes

test/init.test.ts Outdated Show resolved Hide resolved

Update test/init.test.ts

f6036f3

nmigueles commented Jan 9, 2025

View reviewed changes

test/init.test.ts Outdated Show resolved Hide resolved

Update test/init.test.ts

a95e5e2

		Assertion.addMethod('pathExists', function () {
		const path = this._obj

		const exists = existsSync(path)

		const pathBasePath = path.split('/').slice(0, -1).join('/')

Init :: Sanitize name and paths #193

Are you sure you want to change the base?

Init :: Sanitize name and paths #193

Conversation

nmigueles commented Sep 30, 2024 • edited Loading

nmigueles commented Oct 2, 2024 • edited Loading

PalumboN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmigueles commented Dec 2, 2024

fdodino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PalumboN commented Jan 5, 2025

fdodino commented Jan 8, 2025

fdodino commented Jan 10, 2025 • edited Loading

Crear o no crear la carpeta

Caracteres incorrectos en el proyecto o el nombre

Nombres de los archivos wlk, wtest

nmigueles commented Sep 30, 2024 •

edited

Loading

nmigueles commented Oct 2, 2024 •

edited

Loading

fdodino commented Jan 10, 2025 •

edited

Loading