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

✨ Add check for overwrite readme #143

Merged
merged 4 commits into from
Oct 4, 2019
Merged

✨ Add check for overwrite readme #143

merged 4 commits into from
Oct 4, 2019

Conversation

tbetous
Copy link
Contributor

@tbetous tbetous commented Oct 3, 2019

Add check for overwrite readme #140

@tbetous
Copy link
Contributor Author

tbetous commented Oct 3, 2019

Hi @kefranabg , there is no unit test yet, but I would be glad to have your opinion about my implementation. I'll do tests if you are agree with it.

@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #143 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #143   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          24     25    +1     
  Lines         197    206    +9     
  Branches       21     23    +2     
=====================================
+ Hits          197    206    +9
Impacted Files Coverage Δ
src/cli.js 100% <100%> (ø) ⬆️
src/readme.js 100% <100%> (ø) ⬆️
src/ask-overwrite.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a60627c...b0fa1f0. Read the comment docs.

Thomas Betous and others added 2 commits October 3, 2019 10:13
@kefranabg
Copy link
Owner

Implem looks good to me, I think you can go ahead! I'll do a full review ASAP

@tbetous
Copy link
Contributor Author

tbetous commented Oct 3, 2019

Done ! I added tests.

Copy link
Owner

@kefranabg kefranabg left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just have a strange rendering of the emoji 🤔 Could you take a look?
Capture d’écran 2019-10-04 à 10 51 24

const question = {
type: 'list',
message:
'⚠ Readme-md-generator will overwrite your current README.md. Are you sure you want to continue? ',
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
'⚠ Readme-md-generator will overwrite your current README.md. Are you sure you want to continue? ',
'⚠ readme-md-generator will overwrite your current README file. Are you sure you want to continue? ',

const expectedQuestion = {
type: 'list',
message:
'⚠ Readme-md-generator will overwrite your current README.md. Are you sure you want to continue? ',
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
'⚠ Readme-md-generator will overwrite your current README.md. Are you sure you want to continue? ',
'⚠ readme-md-generator will overwrite your current README file. Are you sure you want to continue? ',

*
* @param {Object} args
*/
module.exports = async ({ customTemplatePath, useDefaultAnswers }) => {
if (!(await readme.checkOverwriteReadme())) return
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (!(await readme.checkOverwriteReadme())) return
if (!(await readme.checkOverwriteReadme())) return

@kefranabg
Copy link
Owner

Thanks buddy

@kefranabg kefranabg merged commit 2e12553 into kefranabg:master Oct 4, 2019
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