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.

Sign up for GitHub

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

Jump to bottom

Draft: Refactoring the whole app #7

Open
wants to merge 7 commits into
base: development
Choose a base branch
from
Open

Draft: Refactoring the whole app #7

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff7517d
Add development tools
fabien-renaud Feb 8, 2021
be103d4
Add core and messages
fabien-renaud Feb 8, 2021
b3416c6
core and messages documentation
fabien-renaud Feb 8, 2021
bf923c0
Fix PR comments
fabien-renaud Feb 9, 2021
848c4b8
Fix 2 lines comments
fabien-renaud Feb 9, 2021
0e3a48e
Rename npm validate scripts
fabien-renaud Feb 9, 2021
618afd4
Fix dotenv loading
fabien-renaud Feb 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/src/messages
/src/modules
/src/scripts
/src/tasks
/src/utils
/src/index.js
13 changes: 13 additions & 0 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"extends": ["node", "prettier"],
"plugins": ["prettier"],
"settings": {
"import/resolver": {
"node": {"extensions": [".js"]}
}
},
"rules": {
"import/prefer-default-export": "off",
"prettier/prettier": ["error"]
}
}
3 changes: 1 addition & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
.env
.idea
*.bat
/lib
/node_modules
package-lock.json
lib
6 changes: 6 additions & 0 deletions .huskyrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"hooks": {
"pre-commit": "npm run test",
"pre-push": "npm run test"
}
}
Comment on lines +1 to +6
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be better to put these checks in a workflow file and prevent merge instead of enforcing it on pre-commit since anybody can disable their pre-commit locally and push

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know about GitHub workflow, so I guess I will just leave it to you! I see GitHub hooks as a reminder for the developers to not push or merge broken code, but you're right saying that you can just skip Git hooks. We can remove this feature if it doesn't make senses for you!

Copy link
Collaborator

@Fox-Islam Fox-Islam Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If you create a .yml file in .github/workflows/ with these contents it should pick it up as a job when you next push:

name: Node CI

on:
  push:
    branches:
      - "**"

jobs:
  lint:
    name: Run linters
    runs-on: ubuntu-latest

    steps:
      - name: Check out Git repository
        uses: actions/checkout@v2

      - name: Set up Node.js
        uses: actions/setup-node@v2

      - name: Install Node.js dependencies
        run: npm install

      - name: Run linters
        run: npm run lint

Alternatively (the better solution, imo):

name: Lint

on:
  push:
    branches:
      - "**"

jobs:
  run-linters:
    name: Run linters
    runs-on: ubuntu-latest

    steps:
      - name: Check out Git repository
        uses: actions/checkout@v2

      - name: Set up Node.js
        uses: actions/setup-node@v1
        with:
          node-version: 12

      - name: Install Node.js dependencies
        run: npm install

      - name: Run linters
        uses: wearerequired/lint-action@v1
        with:
          github_token: ${{ secrets.GITHUB_TOKEN }}
          eslint: true
          prettier: true

which integrates with GitHub but we'd need to add a GitHub token into the secrets first and also add the .github directory into the .eslintignore and .prettierignore files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Github Secrets aren't a problem. And there's no point adding .github directory on the .eslintignore and .prettierignore, since we're only running linter on the src folder.
I think we can integrate both git hooks and github workflow: git hooks allows you to automatically validate your code before comiting and pushing your branch, then github workflow force a validation before merging your branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep! Adding the secret is easy, it should just be done before pushing the workflow file (otherwise it'll fail the check)

Maybe, but I'm not sure about it since it would add a redundant dependency on husky, it isn't a particularly large package but if possible we should try to minimise package bloat whenever we can

6 changes: 6 additions & 0 deletions .prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/src/messages
/src/modules
/src/scripts
/src/tasks
/src/utils
/src/index.js
18 changes: 18 additions & 0 deletions .prettierrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"printWidth": 120,
"tabWidth": 4,
"useTabs": false,
"semi": true,
"singleQuote": true,
"quoteProps": "as-needed",
"jsxSingleQuote": true,
"trailingComma": "none",
"bracketSpacing": false,
"jsxBracketSameLine": true,
"arrowParens": "always",
"requirePragma": false,
"insertPragma": false,
"proseWrap": "preserve",
"htmlWhitespaceSensitivity": "ignore",
"endOfLine": "auto"
}
Loading