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 Typer CLI Interface #2

Merged
merged 9 commits into from
May 16, 2024
Merged

Add Typer CLI Interface #2

merged 9 commits into from
May 16, 2024

Conversation

lispandfound
Copy link
Collaborator

This PR is here for a few reasons:

  1. To familiarise the others with how to use typer to generate better help messages and add better bounds checking on CLI arguments.
  2. To demonstrate that we don't need Jenkins for black formatting and instead use GitHub actions
  3. To show how to use Python's newish =pyproject.toml= standard to configure a reproducible virtual environment (see the readme for a PyCharm setup).
  4. To allow you to familiarise yourself with the new NSHM database code and critique it's current implementation.

@sungeunbae
Copy link
Member

Good to see a PR being used as the communication means. Quite enlightening to see new Python packages/technique in action.
A few questions. Does it actually run black on the pushed code( and update the code) ? Does it mean the github repo will be instantly ahead of the local copy, and the user needs to git pull to keep things in sync? (overall mechanism is slightly unclear)

I understand this is still a work in progress and its primary purpose is to share the info. However, if you add a docstring to each function, it will be fantastic.

@lispandfound
Copy link
Collaborator Author

lispandfound commented May 10, 2024

@sungeunbae Just to put what we discussed today in writing for the future. No. It does not format the code. I think this is for the best. Instead it simply checks that the code is black complaint (like what the Jenkins build server currently does). The advantages are two-fold:

  1. It does not require a Jenkins server.
  2. The Black authors publish a GitHub action and yaml file for the action that we don't have to maintain. This is everything that is contained in that file currently:
name: Lint

on: [push, pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - uses: psf/black@stable

The uses: list items in the steps section indicate that the action should run some predefined actions (in this case, the official black github action).

The checks are made at exactly the same time the Jenkins server does, i.e. when you create a pull request as prerequisite for merging into the main branch. For our purpose this is a drop-in replacement for the Jenkins workflow.

nshm_geojson_fault_parser.py Outdated Show resolved Hide resolved
nshm_geojson_fault_parser.py Outdated Show resolved Hide resolved
Copy link

@joelridden joelridden left a comment

Choose a reason for hiding this comment

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

Aside from claudio's comments I approve.

Outside of this PR, but could be good at some stage to have some reading functions for those to be able to access fault / rupture information from the DB without having to understand how to do SQL etc.

@sungeunbae
Copy link
Member

@sungeunbae Just to put what we discussed today in writing for the future. No. It does not format the code. I think this is for the best. Instead it simply checks that the code is black complaint (like what the Jenkins build server currently does). The advantages are two-fold:

  1. It does not require a Jenkins server.
  2. The Black authors publish a GitHub action and yaml file for the action that we don't have to maintain. This is everything that is contained in that file currently:
name: Lint

on: [push, pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - uses: psf/black@stable

The uses: list items in the steps section indicate that the action should run some predefined actions (in this case, the official black github action).

The checks are made at exactly the same time the Jenkins server does, i.e. when you create a pull request as prerequisite for merging into the main branch. For our purpose this is a drop-in replacement for the Jenkins workflow.

Cool - we have no immediate plan to add extra automated tests other than black. I'm supportive of more integration into github eco-system.

@lispandfound lispandfound requested a review from claudio525 May 14, 2024 00:42
@sungeunbae sungeunbae merged commit 0027cc4 into main May 16, 2024
2 checks passed
@sungeunbae sungeunbae deleted the typer_cli branch May 16, 2024 01:16
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.

5 participants