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

New linter. Readme stickers #355

Merged
merged 4 commits into from
Feb 5, 2024
Merged

New linter. Readme stickers #355

merged 4 commits into from
Feb 5, 2024

Conversation

MattWellie
Copy link
Collaborator

Fixes

  • We have previously had incidents where real sample identifiers have been committed to public-facing repositories

Proposed Changes

  • Adds the homebrewed CPG pre-commit hook to check for CPGXXX identifiers in project files
  • Adds some additional Ruff rules, e.g. to prevent iSort clashing
  • Adds some vanity stickers to the project root Readme

Checklist

  • Related Issue created
  • Tests covering new change
  • Linting checks pass

@MattWellie MattWellie requested a review from illusional February 5, 2024 04:47
Copy link
Contributor

@illusional illusional left a comment

Choose a reason for hiding this comment

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

Broadly supportive, left some inline comments but you can take or leave

@@ -113,9 +115,9 @@ of all panels on which they have previously been seen. A gene will be treated as
before, or features in a phenotype-matched panel on which it has not been seen before. i.e.

- if a Gene is promoted to `Green` on the Mendeliome, it will be recorded as `New`, and the prior data will be extended
to show that the gene was seen on the Mendeliome.
to show that the gene was seen on the Mendeliome.
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, just make this one line, and use soft-wrapping in VSCode:

// in the settings.json
"[markdown]": {
    // ... other settings
    "editor.wordWrap": "wordWrapColumn",
    "editor.wrappingIndent": "same",
    "editor.wordWrapColumn": 80,
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one was caused by markdownlint... or ruff. One or the other. On-save this got split off back onto separate lines. I've reformatted all markdown files in the repo to remove artificial line breaks

@@ -283,7 +283,7 @@ def main(
logging.info('Pulling all pedigree members')
pedigree_dicts, ext_lookup = get_pedigree_for_project(project=project)

# endpoint gives list of tuples e.g. [['A1234567_proband', 'CPG12341']]
# endpoint gives list of tuples e.g. [['A1234567_proband', 'CPT12341']]
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to keep the CPG prefix, and make it CPGaaaa so it's clear what the second option is here.

@MattWellie MattWellie merged commit f0412d8 into main Feb 5, 2024
4 checks passed
@MattWellie MattWellie deleted the New-inhouse-linter branch February 5, 2024 06:37
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.

2 participants