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

Chore: Change code samples following the Runkit pattern #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lnfnunes
Copy link

Alteração simples nos códigos de exemplo ("Como utilizar") de forma que se a pessoa simplesmente consiga copiar e colar no Runkit e o código funcione!
Pois o padrão do Runkit é seguindo o nome da lib e neste caso fica cepPromise e não apenas cep como mostrado na imagem abaixo:

Runkit

image

Readme

image

  • PS: Alterei o import também nos testes unitários para manter um padrão 😉

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ad22067 on lnfnunes:npm-runkit into 69aa725 on filipedeschamps:master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ad22067 on lnfnunes:npm-runkit into 69aa725 on filipedeschamps:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ad22067 on lnfnunes:npm-runkit into 69aa725 on filipedeschamps:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ad22067 on lnfnunes:npm-runkit into 69aa725 on filipedeschamps:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ad22067 on lnfnunes:npm-runkit into 69aa725 on filipedeschamps:master.

@coveralls
Copy link

coveralls commented Mar 10, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling de49344 on lnfnunes:npm-runkit into 9bca1c4 on filipedeschamps:master.

@lnfnunes
Copy link
Author

Obs: Aparentemente o e2e está quebrando um teste, porém não é devido a esta implementação e a correção estou entendendo que já esta aberta no PR #139

Copy link
Member

@lucianopf lucianopf left a comment

Choose a reason for hiding this comment

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

@lnfnunes dei uma olhdinha e curti a proposta! 🤘

Fiquei com apenas uma dúvida, qual a necessidade de alterar arquivos no dist tendo em vista que só foi alterado documentação e testes 🤔

Seria possível vc remover essas alterações do seu PR?

const cepPromise = cep('05010000')
expect(cepPromise.then).to.be.a('function')
expect(cepPromise.catch).to.be.a('function')
const lib = cepPromise('05010000')
Copy link
Member

Choose a reason for hiding this comment

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

Hehehe, nesse caso o cepPromise fazia bastante sentido, pq de fato é a promise retornada pela execução da lib hehehe, mas não vejo problema em manter como lib 😬

@lucianopf
Copy link
Member

Obs: Acho que ta falhando por conta de um problema antigo, acho que faz sentido fazer um rebase com a master pra manter tudo atualizadinho, daí deve voltar a passar 😬 🙏

@lnfnunes
Copy link
Author

Valeu @lucianopf, rebaseei e fiz os ajustes! ✌️

@lucianopf
Copy link
Member

Mestres, só pra não deixar vcs perdidos abri uma issue com um planinho de ação pra reorganizar o repo dado a migração pra org do BrasilAPI 😬

#197

@lucianopf
Copy link
Member

Bom dia mestre @lnfnunes !

Primeiramente perdão a demora 😢

Bora seguir com esse PR? Eu tentei fazer o rebase pra vc mas a branch tava protegida e precisarei da sua ajuda 😢

Pode fazer os seguintes passos por favor? 🙏

git remote add upstream https://github.com/BrasilAPI/cep-promise.git
git fetch --all
git rebase upstream/master
git push --force-with-lease

Assim que terminar o push como o CI foi "corrigido" acho que vai passar e podemos seguir com o merge 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants