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

Mi md links Iris Trejo #28

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

Mi md links Iris Trejo #28

wants to merge 7 commits into from

Conversation

IrisFyD
Copy link

@IrisFyD IrisFyD commented Jun 5, 2020

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.

Iris, me agrada que pudiste resolver tu proyecto y cumpliste con el requerimiento. Las observaciones que tengo son sobre la descripción del archivo README.md porque el usuario no tiene una guía practica de como opera la librería y como instalarla, mi sugerencia es completar con multimedia(capturas de pantalla ó videos) de la instalación, descripción y ejecución, por otro lado me percate que el uso de TESTING lo dejaste de lado, lo cual te recomiendo complementar testeando todas tus funciones faltantes con DATOS REALES, usar datos reales es muy útil, observe buenas practicas lo cual es importante resaltar.

Comment on lines +1 to +2
node_modules/
.DS_Store

Choose a reason for hiding this comment

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

esta genial el excluir estas carpetas, añadiría la carpeta assets y package-lock.json

@@ -0,0 +1,81 @@
let fs = require('fs')

Choose a reason for hiding this comment

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

cuando las variable mutan(cambian su valor) se usa let, si no mutan se usa const

Suggested change
let fs = require('fs')
const fs = require('fs')

Comment on lines +4 to +5
let fetch = require ('node-fetch')
let colors = require ("colors")

Choose a reason for hiding this comment

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

Suggested change
let fetch = require ('node-fetch')
let colors = require ("colors")
const fetch = require ('node-fetch')
const colors = require ("colors")



// read file
function readMd (uri){

Choose a reason for hiding this comment

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

Suggested change
function readMd (uri){
function readMd (file){

Choose a reason for hiding this comment

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

define mejor el parametro

// read file
function readMd (uri){
let readString = fs.readFileSync(uri, 'utf-8')
return `${readString}`

Choose a reason for hiding this comment

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

Suggested change
return `${readString}`
return readString

{
"name": "CDMX009-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.

tu descripción tiene que ser funcional y en ingles

Suggested change
"description": "## Preámbulo",
"description": "## Preámbulo",

@@ -0,0 +1,11 @@
const { readMd } = require('../index');

describe('test de funcion 1', () => {

Choose a reason for hiding this comment

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

Suggested change
describe('test de funcion 1', () => {
describe('function testing one', () => {

const { readMd } = require('../index');

describe('test de funcion 1', () => {
test('espero que mi funcion 1 sea una funcion', () => {

Choose a reason for hiding this comment

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

ingles

Suggested change
test('espero que mi funcion 1 sea una funcion', () => {
test('function one response function', () => {

expect(typeof readMd).toBe('function')
})
})
it('Se espera leer un archivo de lectura',() => {

Choose a reason for hiding this comment

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

recomiendo ser más puntual en la descripción

it('Se espera leer un archivo de lectura',() => {
let uri = './otraCosa.md'
expect(typeof readMd(uri)).toBe('string')
})

Choose a reason for hiding this comment

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

El testing es importante, te recomiendo hacer testing de todas tus funciones

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