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 typos, add typos linter to CI #1057

Merged
merged 2 commits into from Oct 14, 2023
Merged

Fix typos, add typos linter to CI #1057

merged 2 commits into from Oct 14, 2023

Conversation

dosisod
Copy link
Contributor

@dosisod dosisod commented Sep 11, 2023

typos is a great tool for finding typos in a varietry of file formats.

I have not tested these changes, though most changes are in the documentation and the docstrings. There are 2 potential API changes: The saftey_buffer arguments for tidy_blacklist_tags and tidy_identity_whitelist_entries have been changed to safety_buffer. If we want to maintain compatibility we could use kwargs and check for the new flag, then the old one.

@dosisod dosisod requested a review from a team as a code owner September 11, 2023 19:02
@briantist briantist self-assigned this Sep 16, 2023
@briantist briantist added dependencies Pull requests that update a dependency file maintenance General technical debt developer experience Developer setup and experience labels Sep 16, 2023
@briantist
Copy link
Contributor

Thanks for this @dosisod ! It's uh.. really something to see all the typos here 😅 I'd like to treat this PR a little differently than most.

First, the one functional change you mentioned, re: saftey_buffer, thanks for specifically raising that. I want to update that parameter specifically in a new PR, and make the change with an aliased parameter and deprecation of the old. Luckily, we have a pattern for doing just that:

Because this will then eventually be a breaking change (in v3.0.0 when the old one is removed) I want to get this out in v2.0.0 so folks have time to make updates.


The other thing I want to do then is 1) add the typos package/workflow in CI, and make the other typo changes themselves.

I want all the typo changes to be in a single commit, so that we can put that commit into .git-blame-ignore-revs for helping blame views: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

We need to do this via at least 2 PRs.

The first PR (this one, probably) can both set up the typo workflow, and fix the typos, each in their respective commits, and I will "Rebase and merge" instead of squash and merge (so we will have to rebase/squash locally to get the 2 commits we want). Alternatively, we could do these steps as separate PRs but I guess we'd have to fix all the typos first so the workflow doesn't fail on them.

The last (second or third) PR would be to add the ignore file with the commit hash containing the typo fixes.


I consider the new PR for saftey_buffer to be high priority and want to get that done soon (if you intend to submit that PR, which is very appreciated, please let me know, if not I will get to it at some point soon), the other steps here... I may have to delay those in favor of doing other things to get v2.0.0 out. I only mention that in advance because I don't want to seem like I'm ignoring them if you or someone else puts up those changes. I will definitely get to them (and your PRs so far are pretty complete and easy to review so there's that too).

dosisod added a commit to dosisod/hvac that referenced this pull request Sep 25, 2023
@dosisod
Copy link
Contributor Author

dosisod commented Sep 25, 2023

@briantist sorry for the delay on this! I just opened a PR for the safety_buffer argument. Once that gets merged I'll rebase and remove the safety_buffer changes (as well as the MFA changes), then make the 2 commits for the CI and typo changes.

@briantist
Copy link
Contributor

No worries at all, I've been short on time myself

@dosisod
Copy link
Contributor Author

dosisod commented Sep 27, 2023

@briantist Ok I rebased and made it so there are 2 commits: The first sets up typos and the related workflows/Makefile, and the second commit updates all the typos.

Also, do we need to make a separate PR to update the .git-blame-ignore-revs files? Assuming you do a normal merge and not a rebase, the git SHA should be the same. If you rebase though the SHA's will be different though, hence the need for another PR. I'm fine with it either way, just thought I'd ask.

@briantist
Copy link
Contributor

@briantist Ok I rebased and made it so there are 2 commits: The first sets up typos and the related workflows/Makefile, and the second commit updates all the typos.

Thanks, I've merged some other things and there are conflicts now, but I'm going to pull this down today and try to resolve all that so we can get this in.

Also, do we need to make a separate PR to update the .git-blame-ignore-revs files? Assuming you do a normal merge and not a rebase, the git SHA should be the same. If you rebase though the SHA's will be different though, hence the need for another PR. I'm fine with it either way, just thought I'd ask.

Regular merge is disabled in the repo and I think we want to keep it that way. From what I could find, GitHub (through the UI) does not support any method that preserves commit hashes, though it is possible if you merge from the CLI and push instead which I don't really want to do.

I'll take care of adding the ignore revs file once we're ready, there might be at least one other commit I want to add to it.

@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

Merging #1057 (937a2c2) into main (1d05fae) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1057   +/-   ##
=======================================
  Coverage   86.99%   86.99%           
=======================================
  Files          64       64           
  Lines        3115     3115           
=======================================
  Hits         2710     2710           
  Misses        405      405           
Files Coverage Δ
hvac/adapters.py 90.35% <ø> (ø)
hvac/api/auth_methods/approle.py 100.00% <ø> (ø)
hvac/api/auth_methods/aws.py 57.86% <ø> (ø)
hvac/api/auth_methods/azure.py 91.48% <ø> (ø)
hvac/api/auth_methods/gcp.py 89.74% <ø> (ø)
hvac/api/auth_methods/github.py 100.00% <ø> (ø)
hvac/api/auth_methods/kubernetes.py 89.79% <ø> (ø)
hvac/api/auth_methods/ldap.py 100.00% <ø> (ø)
hvac/api/auth_methods/okta.py 100.00% <ø> (ø)
hvac/api/auth_methods/radius.py 30.00% <ø> (ø)
... and 6 more

`typos` is a great tool for finding typos in a varietry of file formats.
Copy link
Contributor

@briantist briantist left a comment

Choose a reason for hiding this comment

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

Great job with this, thank you!

@briantist briantist merged commit 6c31f14 into hvac:main Oct 14, 2023
33 checks passed
@briantist briantist added the misc Used as a release-drafter "category" label Oct 14, 2023
@dosisod dosisod deleted the fix-typos branch October 14, 2023 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file developer experience Developer setup and experience maintenance General technical debt misc Used as a release-drafter "category"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants