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

linting + CI #96

Merged
merged 17 commits into from
Oct 31, 2023
Merged

linting + CI #96

merged 17 commits into from
Oct 31, 2023

Conversation

kne42
Copy link
Member

@kne42 kne42 commented Oct 17, 2023

adds linting (including pre-commit) and workflows for running linting and tests:

  • pre-commit
  • frontend tests
  • frontend linting
  • frontend type-check
  • backend linting
  • backend code-style

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #96 (217358b) into main (e82082c) will increase coverage by 0.31%.
Report is 2 commits behind head on main.
The diff coverage is 64.51%.

❗ Current head 217358b differs from pull request most recent head dc34d90. Consider uploading reports for the commit dc34d90 to get more accurate results

@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
+ Coverage   78.52%   78.84%   +0.31%     
==========================================
  Files           7        7              
  Lines         503      501       -2     
==========================================
  Hits          395      395              
+ Misses        108      106       -2     
Flag Coverage Δ
unittests 78.84% <64.51%> (+0.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...oet_data_portal/src/cryoet_data_portal/__init__.py 81.81% <100.00%> (ø)
...yoet_data_portal/src/cryoet_data_portal/_client.py 93.33% <100.00%> (-0.42%) ⬇️
client/python/cryoet_data_portal/tests/conftest.py 100.00% <ø> (ø)
...et_data_portal/src/cryoet_data_portal/_gql_base.py 79.33% <66.66%> (ø)
...yoet_data_portal/src/cryoet_data_portal/_models.py 88.79% <90.00%> (ø)
..._data_portal/src/cryoet_data_portal/_file_tools.py 25.00% <30.76%> (+3.33%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kne42 kne42 marked this pull request as ready for review October 18, 2023 17:46
@@ -46,10 +46,6 @@ exclude = ["tests*"] # exclude packages matching these glob patterns (empty by
[tool.setuptools_scm]
root = "../../.."

[tool.black]
line-length = 120
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be good to retain, as the default line limit of 80 is quite short.

Copy link
Member Author

Choose a reason for hiding this comment

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

i can revert this if you desire

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker: Yes, please, if it isn't too much trouble.

"auth_method": "eks"
},
"terraform_directory": ".happy/terraform/envs/prod",
"task_launch_type": "k8s"
},
"api": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this might be more than linting changes. Pulling in the changes from the main might help fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like some of the keys are sorted differently 🤔 I wonder if prettier or eslint did this

pyproject.toml Show resolved Hide resolved
py-dev-requirements.txt Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@codemonkey800 codemonkey800 left a comment

Choose a reason for hiding this comment

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

overall lgtm! left a few comments 🦙

"auth_method": "eks"
},
"terraform_directory": ".happy/terraform/envs/prod",
"task_launch_type": "k8s"
},
"api": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like some of the keys are sorted differently 🤔 I wonder if prettier or eslint did this

Comment on lines 38 to 43
- name: Install PNPM
uses: pnpm/action-setup@v2
with:
version: 8.9.0
- name: Install dependencies with PNPM
run: pnpm install --frozen-lockfile
Copy link
Contributor

Choose a reason for hiding this comment

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

could we leverage the pnpm cache here? there's an example in the action README: https://github.com/pnpm/action-setup#use-cache-to-reduce-installation-time

jobs:
  tests:
    steps:
      - uses: pnpm/action-setup@v2
        name: Install pnpm
        with:
          version: 8.9.0
          run_install: false

      - name: Get pnpm store directory
        shell: bash
        run: |
          echo "STORE_PATH=$(pnpm store path --silent)" >> $GITHUB_ENV

      - uses: actions/cache@v3
        name: Setup pnpm cache
        with:
          path: ${{ env.STORE_PATH }}
          key: ${{ runner.os }}-pnpm-store-${{ hashFiles('**/pnpm-lock.yaml') }}
          restore-keys: |
            ${{ runner.os }}-pnpm-store-

      - name: Install dependencies
        run: pnpm install

Comment on lines 14 to 18
- repo: https://github.com/pre-commit/mirrors-prettier
rev: v3.0.3
hooks:
- id: prettier
files: frontend
Copy link
Contributor

Choose a reason for hiding this comment

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

can you verify that this doesn't install anything with npm install? I'm looking at the code for this and it looks like it's using language: node which does setup an environment using npm install

Copy link
Member Author

Choose a reason for hiding this comment

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

this does use npm install I would assume. we can circumvent this by deleting it and letting the pnpm lint command run it instead

* main:
  single dataset route (#101)
  refactor live reload UI (#92)
  browse all datasets page (#88)
Comment on lines +20 to +22
- uses: actions/setup-python@v4
with:
python-version: '3.9'
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

technically not, no

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker: We could remove it.

@@ -46,10 +46,6 @@ exclude = ["tests*"] # exclude packages matching these glob patterns (empty by
[tool.setuptools_scm]
root = "../../.."

[tool.black]
line-length = 120
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker: Yes, please, if it isn't too much trouble.

@kne42
Copy link
Member Author

kne42 commented Oct 31, 2023

the workflow is failing for reasons i cannot reproduce locally... id rather just merge it in now and maybe we can fix it later since it's mainly meant as a warning

@kne42 kne42 merged commit b6bb3cf into main Oct 31, 2023
24 of 27 checks passed
@kne42 kne42 deleted the kira/linting-ci branch October 31, 2023 22:25
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