-
Notifications
You must be signed in to change notification settings - Fork 1
Iniciando primeira issue #3
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so proud to see you starting the moenda web app, thank you so much for your work, I've made some comments, feel free to discuss and disagree of them.
Also, before any PR, remember to remove the node_modules folder
| @@ -0,0 +1,122 @@ | |||
| const util = require("../util"); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there the same files of moenda? Isn't better to communicate with the moenda app instead of replicate each file? How do you plan keep them sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is download the moenda app and use the modules exported by the app
| test('ruleFirstSectionStartsWithHx returns object', | ||
| () => { | ||
| const object = mdRoules.ruleFirstSectionStartsWithHx("/home/felipe/fork/Moenda/README.md", "/home/felipe/fork/Moenda/config.js"); | ||
| expect("object").toBe(typeof object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is not better to compare the content instead of the types? What are the advantages doing that?
| @@ -0,0 +1,22 @@ | |||
| var Git = require("nodegit"); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use var, there are some inconsistencies caused by it, also JavaScript doesn't start the variables name with uppercase
Todos arquivos contidos na pasta "backend" são iguais aos que já foram revisados no "Moenda", não necessitando portanto de nova revisão. As implementações que acredito que precisem de revisão são as da pasta "services", "app.js" e "server.js".