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

code review #22

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

Conversation

MarianaOlimpia
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.

Mariana, puedo observar que tu proyecto cumple con el requerimiento y eso habla bien de tu dedicación al proyecto, tengo algunas observaciones, la primera es complementar tu archivo README.md para agregar información sobre la librería, recuerda que el usuario nuevo no sabe como instalar tu librería, si agregas información(capturas de pantalla, videos de instalación, diagrama de flujo) el usuario final tendrá una mejor referencia, otra observación es el uso del testing, porque si tu proyecto siguiera creciendo no sabrías si tus funciones siguen sirviendo, recuerda que parte de lo que hace el testing es automatizar la comprobación de el funcionamiento de tu librería. veo implementadas buenas practicas lo cual habla de tu crecimiento.

.gitignore Outdated
@@ -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.

Me agrada la exclusion de la carpeta node_modules/ agregaria la carpeta de assets y el archivo package-lock.json

Comment on lines +2 to +6
const colors = require('colors');
const path = require('path');
const fs = require('fs');
const markdownLinkExtractor = require('markdown-link-extractor');
const fetch = require('node-fetch');

Choose a reason for hiding this comment

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

el uso de const esta genial

let inputDoc = process.argv[2];
let options = process.argv[3];

const validateArgvExist = (inputFile) => {

Choose a reason for hiding this comment

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

cuando el valor muta se implementa let

Suggested change
const validateArgvExist = (inputFile) => {
let validateArgvExist = (inputFile) => {

const readFiles = (inputFile) => {
fs.readFile(inputFile, 'utf8', (err, data) => {
if (err) {
console.log(err);

Choose a reason for hiding this comment

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

Esto se pudo ver mejor con una promesa

Suggested change
console.log(err);
(res=>{
then
catch

Comment on lines +32 to +54
const selectOption = async (inputFile) => {
let links = markdownLinkExtractor(inputFile);
switch (options) {
case '--validate': {
validateLinks(links);
break;
}
case '--stats': {
stats(links);
break;
}
case '--validate--stats':
case '--stats--validate': {
await stats(links);
await validateLinks(links);
break;
}
default: {
getLinks(links);
break;
}
}
};

Choose a reason for hiding this comment

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

Me agrada el uso de la sentencia de control

console.log('Broken Links: '.brightRed + brokenLinks.length);
}

validateArgvExist(inputDoc);

Choose a reason for hiding this comment

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

ten cuidado con la invocación

Suggested change
validateArgvExist(inputDoc);
validateArgvExist(inputDoc);

prueba.md Outdated
@@ -0,0 +1,7 @@
# Archivo de pruebas

Choose a reason for hiding this comment

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

este archivo juto con las imágenes deberían de ir en una carpeta especial llamada assets

{
"name": "cdmx009-mdlinks",
"version": "1.0.0",
"description": "CDMX009 MDLinks Mariana Olimpia",

Choose a reason for hiding this comment

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

La descripción tiene que ser sobre el uso de la librería ademas tendría que estar en ingles

Suggested change
"description": "CDMX009 MDLinks Mariana Olimpia",
"description": "Library for read md files for validate and stats",

prueba.txt Outdated
@@ -0,0 +1 @@
TEXTO DE PRUEBA

Choose a reason for hiding this comment

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

esto debe de estar en assets

@@ -0,0 +1,8 @@
const { validateArgvExist, validateExt, readFiles, selectOption, getLinks, validateLinks, stats } = require('../app')

Choose a reason for hiding this comment

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

veo la invocación pero no la implementación

Suggested change
const { validateArgvExist, validateExt, readFiles, selectOption, getLinks, validateLinks, stats } = require('../app')
const { validateArgvExist, validateExt, readFiles, selectOption, getLinks, validateLinks, stats } = require('../app')

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