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 .editorconfig #24

Merged
merged 5 commits into from Oct 9, 2019

Conversation

@donaldducky
Copy link
Contributor

commented Oct 8, 2019

fixes #22

fixes #22
@donaldducky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

Running this tool:
https://github.com/editorconfig-checker/editorconfig-checker.javascript

Produces the following output:
Screen Shot 2019-10-08 at 6 12 29 PM

All of them are easily fixable except for db/seeds/data/libraries.yml.
Those lines 108-110 are formatted that way for the python code in there.

I'm not too sure on how to fix that issue atm (aside from ignoring it explicitly).

@MattIPv4

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

Seed data can be ignored as it’s auto generated — this looks great, I think it’d be awesome to add a commit here that fixes these errors and maybe add this checker to the test suite, though you can do that in a different PR if you’d prefer (or not at all).

@MattIPv4 MattIPv4 referenced this pull request Oct 8, 2019
Ignore db seeds as it is auto-generated.
.editorconfig Outdated Show resolved Hide resolved
@donaldducky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

For the ci portion, I can add an npm run command: "lint:editorconfig": "editorconfig-checker"

In the .github/workflows/ci.yml should I just add a step after npm install?

I haven't worked with the workflows before but I guess it really depends when/how you want this to fail the build.

If you want to force the build to fail with this check, I'd suggest maybe moving npm install to the top and then running this check. If those pass, do the db creation and the rest of the ci.

@MattIPv4

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

I think the best bet would actually to be to integrate it into the mocha test suite — I realise it’s more work but it means it’ll get checked whenever and where ever tests get run, not just ci.

@MattIPv4

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

If you want to split that into a separate PR and merge this first, happy to do that too.

@donaldducky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

I think it makes sense to include it as a part of this so the editorconfig is enforced as soon as it's merged in.

I'm looking at: deploy/src/test.js

Would you want something like this?

    // Run the test suite
    try {
        out = await run('npm run lint:editorconfig');
        out += await run('npm run test -- --skip-lighthouse --reporter dot');
    } catch (e) {
        err = e;
    }
@MattIPv4

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

Not there, those only get run when I deploy to staging or production — it’ll want to live in the /test directory as a mocha test file. As it isn’t specific to any part of the code base, it can live at the root like the lighthouse test file. Either it can make a call to the npm command as the “test” using exec, or if there’s programmatic integration available for the tool that’d be best.

@MattIPv4

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

It’ll probably be a super basic mocha file:

describe editorconfig
it passes
run exec or integration
expect status to be okay

@donaldducky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

Ok, I'll have a look in a little bit.

I'll add a test file in that folder called editorconfig with the describe block and it'll have to shell out to exec. I don't see a programmatic interface for that tool, unfortunately :(

@MattIPv4

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

Awesome, sounds like a plan, tyvm.

@donaldducky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

@MattIPv4 I think this is good now, have the test in place. It passed in CI.

Here's what the output looks like when it fails:

Screen Shot 2019-10-08 at 8 08 40 PM

Tested it using the following command:
./node_modules/mocha/bin/mocha test/editorconfig.js

@MattIPv4 MattIPv4 self-requested a review Oct 9, 2019
Copy link
Member

left a comment

Lgtm.

@MattIPv4 MattIPv4 merged commit d6a0f25 into botblock:node Oct 9, 2019
2 checks passed
2 checks passed
build
Details
WIP Ready for review
Details
@donaldducky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

Cool, totally forgot to remove the expect and secret.

Curious about the done => {} to function (done) {} change.
Guess it's just for scoping this.retries and this.slow?

Would it be preferable to change all the tests to use that syntax rather than the arrow function? Or are you only using the function syntax when you need access to this?

@MattIPv4

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Yeah, only using function where I need access to this :)

drdrjojo added a commit to drdrjojo/BotBlock.org that referenced this pull request Oct 9, 2019
Add .editorconfig (botblock#24)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.