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

Fix migration clippy warns #442

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

GracefulLemming
Copy link
Contributor

Part of a series of PRs incorporating Clippy checks into CI. Since the desired end behavior is a CI fail when cargo clippy checks fail or warn, existing clippy warns must first be resolved. This PR focuses on resolving semantically and structurally impactful changes in /migrations. Additional PRs will be created for logical changes to types and admin-event-handlers, as well as an additional PR for CI integration.

This PR builds on changes from #441.

Note

This PR does not resolve all clippy warns nor does it implement clippy into CI.

Work completed:

  1. All work in Fix non-structural Clippy warns #441.
  2. Removed unused validation module. Discussed further below.
  3. Removed some unused code. Some variables in spreadsheet parsing logic aren't used but are worth keeping as named variables for clarity. These have been appropriately prefixed with a _ character. For example, see migration/src/lexical.rs:385.
  4. Added some structuring for grammatical reference spreadsheet functions. Discussed further below.
  5. Converted some Strings to strs.

Removing validation

The validation module was added in #236 to reduce excessive SSH connection wait times. It was removed from GitHub Actions Workflows in #291 and #349. It has remained unused in the time since those PRs were merged. Based on this history, there is no compelling reason to preserve this unused code.

Removing validation reduces needless duplication and helps developers. The code in validation seems to be largely copy-pasted from migration/src/main.rs. This duplicates some of our most terse and logically complex Rust code, adding a major hurdle to regular maintenance and developer education. Additionally, as types are updated, migration/main.rs is updated regularly while validation is not, creating massive discrepancies in what should otherwise be similar programs.

The DAILP Translation Interface (TI) makes aspects of validation obsolete. validation and migration/src/main.rs were both written at a time where data migration was a regular step in DAILP's workflows. With the creation of DAILP TI, bulk ingestion from spreadsheets now only happens on particular occasions. As a result, all of migration is in need of restructuring and validation in its current form will only become more outdated. In place of validation, we may want to add a dry-run migration that reads from google sheets and writes to a preview file instead of the database.

Structuring for grammatical references

Functions in migration/src/early_vocab.rs and migration/src/lexical.rs parse spreadsheets containing data from dictionaries and grammars of Cherokee and store that data in the database. Some of these functions triggered Clippy's too many arguments warning. Upon further inspection, it became clear that functions with too many arguments also had arguments whose function or meaning were unclear. To resolve both of these issues, I created the EarlyVocabMetadata and DFSheetMetadata structs to represent key information about early vocabularies spreadsheet structure and Durbin Feeling grammar/dictionary spreadsheet structure. Further documentation would be helpful in explaining what this metadata represents and how it is used.

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for dailp canceled.

Name Link
🔨 Latest commit f636ead
🔍 Latest deploy log https://app.netlify.com/sites/dailp/deploys/6761f4ea7a8eec0008d0ea07

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.

1 participant