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

validation was performed #30

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

Conversation

castillojessica
Copy link

No description provided.

Copy link

@reloadercf reloadercf left a comment

Choose a reason for hiding this comment

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

Me agrada la Logica que implementaste, puedes mejorar lo siguiente:
1)la indentation de tu código y la definición del nombre utilizado en variables y funciones,
2)Si vas a documentar tu código se en ingles y solo utiliza palabras clave
3)Saber de manera concreta donde implementar console.log() y donde utilizas el mensaje de la respuesta.
4)Te recomendaria utilizar de forma activa GitHub, para plasmar el proceso de construcción
En general lo que más rescato es el pensamiento para abordar el problema y las soluciones propuestas.

@@ -0,0 +1 @@
node_modules/

Choose a reason for hiding this comment

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

Esta genial discriminar la carpeta node_modules/ te recomiendo excluir el archivo package-lock.json

@@ -1,355 +1,10 @@
# Markdown Links
# Hola amiguitos

Choose a reason for hiding this comment

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

Tus comentarios del readme tienen que ser más formales

Suggested change
# Hola amiguitos
# Bienvenidx a la libreria....

[Modificación de link](https://www.youtube.com/wach?v=fBNz5xF-Kx488)

Choose a reason for hiding this comment

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

recomiendo que tu Readme tenga recursos para el usuario final y el propósito de estos recursos sea ayudar en la instalación de la librería y tener una mejor documentación

Comment on lines +1 to +3
const fs = require('fs')
const fetch = require('node-fetch')
const colors = require('colors')

Choose a reason for hiding this comment

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

Esto esta genial, el uso de const para las variables que el valor no muta

Comment on lines +7 to +14
function getContentString() {

let index = process.argv.indexOf("--file")
if (index < 0) return console.log("You need to use a valid uri flag --file")
let uri = process.argv[index + 1]
let string = fs.readFileSync(uri, 'utf8')
return string
}

Choose a reason for hiding this comment

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

La definición de tus funciones debería contestar a la pregunta ¿qué hace la función?

Suggested change
function getContentString() {
let index = process.argv.indexOf("--file")
if (index < 0) return console.log("You need to use a valid uri flag --file")
let uri = process.argv[index + 1]
let string = fs.readFileSync(uri, 'utf8')
return string
}
function getContentStringArgv() {
let index = process.argv.indexOf("--file")
if (index < 0) return console.log("You need to use a valid uri flag --file")
let uri = process.argv[index + 1]
let string = fs.readFileSync(uri, 'utf8')
return string
}

//console.log("las promesas de tu ex: ", promises)
};

let main = async() => { // la encargada de controlar lo que pasa en el programa (imperativo ó programacion imperativa)

Choose a reason for hiding this comment

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

Esto esta genial

Comment on lines +81 to +84
if (shouldShowTotals > -1) {
// contar
// muestro results
}

Choose a reason for hiding this comment

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

Esto no tiene sentido, si shouldShowTotals > -1 NO HACE NADA

Suggested change
if (shouldShowTotals > -1) {
// contar
// muestro results
}
if (shouldShowTotals > -1) {
// contar
// muestro results
}

@@ -0,0 +1,3 @@
#!/usr/bin/env node

let { main } = require("./app.js")

Choose a reason for hiding this comment

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

Suggested change
let { main } = require("./app.js")
const { main } = require("./app.js")

Choose a reason for hiding this comment

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

cuando el valor no muta debería de ser const

{
"name": "mdlinks",
"version": "1.0.0",
"description": "## Preámbulo",

Choose a reason for hiding this comment

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

mejora la descripción y te recomiendo que fuese en ingles

"mdlinks": "cli.js"
},
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"

Choose a reason for hiding this comment

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

La sección de scripts debe contener las herramientas como Jest

Suggested change
"test": "echo \"Error: no test specified\" && exit 1"
"test": "jest"

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