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

Warn override #25

Merged
merged 10 commits into from
Apr 24, 2018
Merged

Warn override #25

merged 10 commits into from
Apr 24, 2018

Conversation

jlengrand
Copy link
Collaborator

Fixes #17 .

Adds a warning message if the user tries to configure mergify again

@jlengrand jlengrand changed the title Warn override WIP : Warn override Apr 21, 2018
@jlengrand
Copy link
Collaborator Author

Will add unit tests

@coveralls
Copy link

coveralls commented Apr 21, 2018

Pull Request Test Coverage Report for Build 38

  • 15 of 15 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.1%) to 96.0%

Totals Coverage Status
Change from base Build 33: 1.1%
Covered Lines: 48
Relevant Lines: 50

💛 - Coveralls

Julien Lengrand-Lambert added 3 commits April 21, 2018 15:20
@jlengrand jlengrand changed the title WIP : Warn override Warn override Apr 21, 2018
@jlengrand
Copy link
Collaborator Author

And now with unit tests


const configure = async() => {
if(await checkConfigExists()){
console.log(chalk.red.bold('⚠️ Mergify is already configured. Configuring again will override the existing file.'));
Copy link
Owner

Choose a reason for hiding this comment

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

Can you use the logger for this. It's located at lib/utils/logger


async function checkConfigExists() {
try {
const configFileName = `${__dirname}/../../../.config`;
Copy link
Owner

Choose a reason for hiding this comment

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

Since this is the third time we're getting this path maybe we should put it somewhere else.
I'm thinking a function getConfigPath() would do good here.
Then also readConfig and writeConfig should be updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I was thinking the same. Maybe even actually have a single config module with wrte/read and check in the same file / folder.

@@ -0,0 +1,27 @@
const {checkConfigExists} = require('./index.js');
Copy link
Owner

Choose a reason for hiding this comment

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

There should be spaces surrounding the require like so:

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's add linting per default as a pre commit hook then :)

@@ -0,0 +1,7111 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

Because I've decided to use yarn I don't want to have package-lock files in the repo.
It's confusing and unclear which lockfile is the main source of truth.

@jlengrand
Copy link
Collaborator Author

@RamonGebben all comments processed

@jlengrand jlengrand merged commit e6190d9 into RamonGebben:master Apr 24, 2018
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.

3 participants