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 some typos #692

Closed
wants to merge 2 commits into from
Closed

Fix some typos #692

wants to merge 2 commits into from

Conversation

cmdcolin
Copy link
Contributor

Typos found using https://github.com/crate-ci/typos (a very nice tool if you ask me!). I also saw #691 and was motivated to run it across codebase.

The typo checking could also become a part of a github workflow on this repo, but that is not added by this PR

@github-actions
Copy link

Changed PDFs as of c8e0118: CRAMv2.1 (diff), CRAMv3 (diff), SAMtags (diff), VCFv4.4.draft (diff), crypt4gh (diff).

@jkbonfield
Copy link
Contributor

jkbonfield commented Jan 3, 2023

Thank you for these. Generally it's best to produce separate PRs for the different groups - Refget, crypt4gh, fileformats (SAM,BAM,CRAM,VCF,BCF). The reason is that different maintainers are involved. The issues look trivial though so not really contentious.

We have a File Formats meeting this afternoon so can discuss then, but we may just cherry pick appropriate chunks related to us and leave the other groups to do the same. Will decide later.

@cmdcolin
Copy link
Contributor Author

rebased onto latest master. unsure why CI failed though

@cmdcolin
Copy link
Contributor Author

similar CI error on https://github.com/samtools/hts-specs/pull/684/files

@jkbonfield
Copy link
Contributor

I've been seeing similar CI failures recently. I'm not sure on the cause, but maybe @jmarshall will know?

It's failing on this:

! Undefined control sequence.
\@date ->\headdate 

\headdate is coming from new/*.ver files, generated by scripts/genversion.sh. I see in the logs:

mkdir new
scripts/genversion.sh CRAMv2.1.tex img/CRAMFileFormat2-1-fig001.png img/CRAMFileFormat2-1-fig002.png img/CRAMFileFormat2-1-fig003.png img/CRAMFileFormat2-1-fig004.png img/CRAMFileFormat2-1-fig005.png img/CRAMFileFormat2-1-fig006.png img/CRAMFileFormat2-1-fig007.png > new/CRAMv2.1.ver
fatal: detected dubious ownership in repository at '/github/workspace'
To add an exception for this directory, call:

	git config --global --add safe.directory /github/workspace
fatal: detected dubious ownership in repository at '/github/workspace'
To add an exception for this directory, call:

latexmk --pdf --pdflatex='pdflatex'  --output-directory=new CRAMv2.1.tex
	git config --global --add safe.directory /github/workspace

So it's possible this change is due to some github/latex update which has gained additional security checks.

@jkbonfield
Copy link
Contributor

Closing this as I refactored it.

I split your commits into separate CRAM, SAM, VCF, RefGet and Crypt4GH commits. I then merged the CRAM and SAM ones, of which I am maintainer, and made PRs for the others. See #698 #699 and #700.

This may seem like pointless red tape, but we have different maintainers for each specification and although the typos are really obvious and correct to fix (thank you!), there may be other things the maintainers wish to do while merging the PR relevant to their specification. For example they may wish to check other versions of their documents. It's also just politeness I think. I'm sure @d-cameron wouldn't mind me just pushing the VCF one, but I've not had any recent involvement in RefGet or Crypt4GH.

Ping @d-cameron, @daviesrob and @andrewyatz for their minimal fixes.

PS. Through patch and git-am magic you're still the author of all these commits. :)

@jkbonfield jkbonfield closed this Feb 2, 2023
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.

None yet

2 participants