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

Init :: Sanitize name and paths #193

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"cytoscape": "^3.15.2",
"express": "^4.18.1",
"globby": "^11.0.4",
"lodash": "^4.17.21",
Copy link
Contributor

Choose a reason for hiding this comment

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

Uhhh polémico 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

Esto se usa para las funciones camelCase y kebabCase.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

"loglevel": "^1.8.1",
"p5": "^1.8.0",
"pkg": "^5.8.1",
Expand All @@ -76,6 +77,7 @@
"@types/cli-box": "^6.0.4",
"@types/cytoscape": "^3.19.7",
"@types/express": "^4.17.20",
"@types/lodash": "^4.17.9",
"@types/mocha": "^10.0.7",
"@types/node": "^18.14.5",
"@types/p5": "^1.7.1",
Expand All @@ -94,4 +96,4 @@
"ts-node": "10.9.1",
"typescript": "~5.5.1"
}
}
}
26 changes: 15 additions & 11 deletions src/commands/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ import logger from 'loglevel'
import { existsSync, writeFileSync } from 'node:fs'
import { basename, join } from 'node:path'
import { userInfo } from 'os'
import { ENTER, createFolderIfNotExists } from '../utils'
import { ENTER, createFolderIfNotExists, sanitizeName } from '../utils'
import { PROGRAM_FILE_EXTENSION, TEST_FILE_EXTENSION, WOLLOK_FILE_EXTENSION } from 'wollok-ts'
import kebabCase from 'lodash/kebabCase'
import { execSync } from 'node:child_process'

export type Options = {
Expand Down Expand Up @@ -35,24 +36,27 @@ export default function (folder: string | undefined, { project: _project, name,
}

// Creating files
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'
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

const exampleFilename = sanitizeName(exampleName)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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"

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)


const wollokDefinitionFile = `${exampleFilename}.${WOLLOK_FILE_EXTENSION}`
Copy link
Contributor

Choose a reason for hiding this comment

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

ver comentario más abajo exampleName

logger.info(`Creating definition file ${wollokDefinitionFile}`)
writeFileSync(join(project, wollokDefinitionFile), wlkDefinition)

if (!noTest) {
const testFile = `test${capitalizeFirstLetter(exampleName)}.${TEST_FILE_EXTENSION}`
const testFile = `test${capitalizeFirstLetter(exampleFilename)}.${TEST_FILE_EXTENSION}`
Copy link
Contributor

Choose a reason for hiding this comment

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

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

logger.info(`Creating test file ${testFile}`)
writeFileSync(join(project, testFile), testDefinition(exampleName))
writeFileSync(join(project, testFile), testDefinition(exampleFilename))
}

if (game) {
const gameFile = `main${capitalizeFirstLetter(exampleName)}.${PROGRAM_FILE_EXTENSION}`
const gameFile = `main${capitalizeFirstLetter(exampleFilename)}.${PROGRAM_FILE_EXTENSION}`
Copy link
Contributor

Choose a reason for hiding this comment

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

lo mismo acá: es exampleName

logger.info(`Creating program file ${gameFile}`)
writeFileSync(join(project, `${gameFile}`), gameDefinition(exampleName))
writeFileSync(join(project, gameFile), gameDefinition(exampleFilename))
Copy link
Contributor

Choose a reason for hiding this comment

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

lo mismo: exampleName es lo que yo quiero. Bien con el ${} que estaba al ñudo.

}

logger.info('Creating package.json')
writeFileSync(join(project, 'package.json'), packageJsonDefinition(project, game))
writeFileSync(join(project, 'package.json'), packageJsonDefinition(name ?? project, game))
Copy link
Contributor

Choose a reason for hiding this comment

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

mmm... no, yo quiero project, name es otra cosa


if (!noCI) {
logger.info('Creating CI files')
Expand All @@ -75,7 +79,7 @@ export default function (folder: string | undefined, { project: _project, name,
}

// Finish
logger.info(green('✨ Project succesfully created. Happy coding!'))
logger.info(green('✨ Project successfully created. Happy coding!'))
Copy link
Contributor

Choose a reason for hiding this comment

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

bien por el typo corregido!

}


Expand Down Expand Up @@ -131,7 +135,7 @@ program PepitaGame {
`

const packageJsonDefinition = (projectName: string, game: boolean) => `{
"name": "${basename(projectName)}",
"name": "${kebabCase(basename(projectName))}",
Copy link
Contributor

Choose a reason for hiding this comment

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

ahí tenemos una biblioteca para kebabCase

"version": "1.0.0",
${game ? assetsConfiguration() : ''}"wollokVersion": "4.0.0",
PalumboN marked this conversation as resolved.
Show resolved Hide resolved
"author": "${userInfo().username}",
Expand Down
14 changes: 13 additions & 1 deletion src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import globby from 'globby'
import logger from 'loglevel'
import path, { join } from 'path'
import camelCase from 'lodash/camelCase'
import { getDataDiagram, VALID_IMAGE_EXTENSIONS, VALID_SOUND_EXTENSIONS } from 'wollok-web-tools'
import { buildEnvironment, Environment, getDynamicDiagramData, Interpreter, Package, Problem, validate, WOLLOK_EXTRA_STACK_TRACE_HEADER, WollokException } from 'wollok-ts'
import { ElementDefinition } from 'cytoscape'
Expand Down Expand Up @@ -173,11 +174,22 @@
process.exit(13)
}

// ══════════════════════════════════════════════════════════════════════════════════════════════════════════════════
// SANITIZING
// ══════════════════════════════════════════════════════════════════════════════════════════════════════════════════

/**
* Sanitizes project name to allow it to be used as a filename and as a module name.
* If it doesn't start with a lowercase letter or a '_', it adds '_' at the beggining.
* 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, '_'))

// ══════════════════════════════════════════════════════════════════════════════════════════════════════════════════
// DYNAMIC DIAGRAM
// ══════════════════════════════════════════════════════════════════════════════════════════════════════════════════

export function getDynamicDiagram(interpreter: Interpreter, rootFQN?: Package): ElementDefinition[] {
const objects = getDynamicDiagramData(interpreter, rootFQN)
return getDataDiagram(objects)
}
}

Check warning on line 195 in src/utils.ts

View workflow job for this annotation

GitHub Actions / test

Newline not allowed at end of file
2 changes: 1 addition & 1 deletion test/assertions.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ElementDefinition } from 'cytoscape'
import { existsSync } from 'fs'
import { existsSync, lstatSync, readdirSync } from 'fs'

Check warning on line 2 in test/assertions.ts

View workflow job for this annotation

GitHub Actions / test

'lstatSync' is defined but never used

Check warning on line 2 in test/assertions.ts

View workflow job for this annotation

GitHub Actions / test

'readdirSync' is defined but never used

type ElementDefinitionQuery = Partial<ElementDefinition['data']>

Expand Down
52 changes: 52 additions & 0 deletions test/init.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,22 @@ describe('testing init', () => {
expect(join(customFolderProject, '.gitignore')).to.pathExists()
})


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')
})

it('should skip the initialization of a git repository if notGit flag es enabled', async () => {
init(undefined, { ...baseOptions, noGit: true })

Expand All @@ -119,6 +135,42 @@ describe('testing init', () => {
expect(join(project, GITHUB_FOLDER, 'ci.yml')).to.pathExists()
expect(join(project, 'README.md')).to.pathExists()
expect(join(project, '.gitignore')).to.pathExists()
nmigueles marked this conversation as resolved.
Show resolved Hide resolved
})

it('should normalize imports and filenames', async () => {
init(undefined, { ...baseOptions, name: 'Pepita Game', game: true })

const wollokDefinitionFile = join(project, 'pepitaGame.wlk')
const wollokMainFile = join(project, 'mainPepitaGame.wpgm')
const wollokTestFile = join(project, 'testPepitaGame.wtest')

expect(wollokDefinitionFile).to.pathExists()
expect(wollokMainFile).to.pathExists()
expect(wollokTestFile).to.pathExists()

// Assert content of files
const mainFileContent = readFileSync(wollokMainFile, 'utf8')
expect(mainFileContent).to.include('import pepitaGame.pepita')
const testFileContent = readFileSync(wollokTestFile, 'utf8')
expect(testFileContent).to.include('import pepitaGame.pepita')
})

it('should sanitize especial characters', async () => {
init(undefined, { ...baseOptions, name: 'Some random Game :) !', game: true })

const wollokDefinitionFile = join(project, 'someRandomGame.wlk')
const wollokMainFile = join(project, 'mainSomeRandomGame.wpgm')
const wollokTestFile = join(project, 'testSomeRandomGame.wtest')

expect(wollokDefinitionFile).to.pathExists()
expect(wollokMainFile).to.pathExists()
expect(wollokTestFile).to.pathExists()

// Assert content of files
const mainFileContent = readFileSync(wollokMainFile, 'utf8')
expect(mainFileContent).to.include('import someRandomGame.pepita')
const testFileContent = readFileSync(wollokTestFile, 'utf8')
expect(testFileContent).to.include('import someRandomGame.pepita')
expect(getResourceFolder()).to.be.undefined
nmigueles marked this conversation as resolved.
Show resolved Hide resolved
})

Expand Down
Loading