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

CR OK #44

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

CR OK #44

wants to merge 14 commits into from

Conversation

EstherManrique
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 parece que tu código tiene buenas practicas, lo que puedes mejorar es: el uso de .gitignore, también puedes mejorar la forma en como defines algunas funciones, me gusta mucho la separación de las responsabilidades que tiene cada función y que son independientes una de la otra (esto es modulaoización)

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

Esto es una muy buena practica, todos los proyectos debes tener el archivo .gitignore para excluir elementos como node_modules, recomiendo excluir package-lock.json

@@ -0,0 +1,3 @@
{

Choose a reason for hiding this comment

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

Este archivo debes excluirlo en el .gitignore

@@ -12,344 +12,40 @@ Estos archivos `Markdown` normalmente contienen _links_ (vínculos/ligas) que
muchas veces están rotos o ya no son válidos y eso perjudica mucho el valor de

Choose a reason for hiding this comment

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

Te recomiendo que tu archivo README tenga información de tu librería pensada para cualquier usuario que visite el repositorio, puedes agregar capturas de pantalla ó videos sobre la instalación.

@@ -0,0 +1,7 @@
- [Asíncronía en js](https://carlosazaustre.com/manejando-la-asincronia-en-javascript/)

Choose a reason for hiding this comment

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

este archivo debe de estar en la carpeta con nombre assets y a su vez excluida en .gitignore

@@ -0,0 +1,14 @@
[Markdown](https://es.wikipedia.org/wiki/Markdown) es un lenguaje de marcado

Choose a reason for hiding this comment

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

Traslada a la carpeta assets

@@ -0,0 +1,16 @@
1. Debemos poder leer un archivo.

Choose a reason for hiding this comment

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

agregar a assets y excluir

@@ -0,0 +1,3 @@
# PRBANDO ARCHIVO SIN LINKS

Choose a reason for hiding this comment

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

agregar a assets y excluir

test.spec.js Outdated
});
});

describe('readFile', () =>{

Choose a reason for hiding this comment

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

Suggested change
describe('readFile', () =>{
describe('getUri', () =>{

expect(typeof readFile).toBe('function');
});
});

Choose a reason for hiding this comment

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

puedes tener el siguiente test

Suggested change
describe('readFile', () => {
it('Get all text of the file', () => {
let functionReadFile=readFile("README.md")
expect(typeof functionReadFile).toBe('string');
});
});

test.spec.js Outdated

describe('validateLinks', () => {
it('Should return an status 200 if the link is OK', () => {
expect(result.status).toBe(200);

Choose a reason for hiding this comment

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

Testear la función que genera el CLI

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