GSA / 889-tool

Web service for determining 889 compliance of vendors
1 stars 0 forks source link

Documentation and script updates #114

Closed eric-gade closed 8 months ago

eric-gade commented 9 months ago

What

This PR is part of a documentation and code review of the 889-tool. The primary purpose is to ensure that the documentation (setup, development, and test instructions) work for someone new to the tool's development.

Summary of major changes

README

Dev script changes

I have made some modifications to the top-level package.json npm scripts that might make initial setup and running a bit easier.

The top-level scripts now specify dev:frontend and dev:backend for running each part of the application in development mode. This allows the new script dev, which runs both together at the same time, as a convenience.

I've also added test:backend and test:frontend to easily run both kinds of tests.

The frontend test command has been modified so that it runs without watch, and exits correctly once the suite has finished. Note for now that these frontend test are just the unit tests (see the Remaining Issues below).

Remaining Issues

There are some miscellaneous issues for which I haven't made changes yet, because I wanted to get feedback from the team first:

Questions about README structure

Currently, the top-level README mostly relates to the FastAPI part of the application, even with the edits in this PR. There is a separate README for the frontend in the front-end subdirectory. For the moment I've simply made reference to it from the parent document, but there are other options to consider:

  1. Keep things as-is in these changes, with linked references to the frontend README file at the top level;
  2. Create a new top-level README that only provides basic information, moving the current README into the samtools subdirectory and linking as needed to each frontend/backend specific document;
  3. Combining both READMEs into one (longer) document

Additionally, there appears to be a lot of extraneous information about the FastAPI application in the current README (for example, documenting the structure of code in the repository and the schema of the api). These extra bits of information might be better suited to a separate file in the samtools subdirectory. As someone coming aboard the project, I think it's most important to get a brief overview, then jump right to building/running/testing instructions.

Tests

Github Actions/CI

There currently are no github actions related to testing, linting, or accessibility checks (pa11y). Do we want to address those now, or are they not necessary at this point?

mark-meyer commented 8 months ago

This is super helpful! I made a couple changes to the linting to just ignore the USWDS files since we are not going to edit them — and fixed some of the genuine issues it was pointing out. The linter is not complaining anymore — but I think I stepped on your work and now we have merge conflict. I'll look at the that tomorrow — we will probably want to crib your eslint config files.

I also added a github workflow to run the linting and frontend tests. I'll look at more tomorrow — Thank you!

mark-meyer commented 8 months ago

@eric-gade I made one commit to this to bump up eslint one more minor version and to add a rule so to ignore single-word Vue components like Alert for now. Staging will now run front and tests and linter checks and they should be passing.

eric-gade commented 8 months ago

@mark-meyer @weiwang-gsa I've gone ahead and done what we discussed for this repo, namely:

The README got kind of long, even with cuts, so I added a few "callout" links right at the top. LMK what you think