Skip to content

spell: turn on pre-commit auto spell checker #1184

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dmyger
Copy link
Collaborator

@dmyger dmyger commented Jul 8, 2025

Turning on automatic static code analyzer: check words misspell for code files.

Closes #TNTP-3105

Come with second part in #1185

  • Well-written commit messages (see documentation how to write a commit message)

@dmyger dmyger force-pushed the dmyger/tntp-3105_turn_on_spell_checker_for_code branch from 370cdba to c04ce9f Compare July 10, 2025 12:09
@dmyger dmyger changed the title spell: turn on precommit spell checker for Python spell: turn on pre-commit auto spell checker Jul 10, 2025
@dmyger dmyger requested a review from oleg-jukovec July 10, 2025 12:11
@dmyger dmyger marked this pull request as ready for review July 10, 2025 12:11
@dmyger dmyger mentioned this pull request Jul 10, 2025
@dmyger dmyger force-pushed the dmyger/tntp-3105_turn_on_spell_checker_for_code branch from 2b4a206 to 59e26ec Compare July 10, 2025 13:03
@dmyger dmyger added the full-ci Enables full ci tests label Jul 10, 2025
@dmyger dmyger force-pushed the dmyger/tntp-3105_turn_on_spell_checker_for_code branch 3 times, most recently from b4547b7 to 20a96d7 Compare July 16, 2025 10:27
SKIP: golangci-lint-full
with:
extra_args: --all-files --from-ref=${{ env.BASE_BRANCH }} --to-ref=HEAD --hook-stage=manual
# - name: pre-commit checks (diff)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks a bit strange that this PR disables this job and the next PR (#1185) enables it back and it is the only change #1185 does. Why don't we just keep it intact here, so #1185 would not be needed at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checking “all” files (including old code) is done with lightened requirements, but even this required minor changes. However, any changes are treated as “new” code. The purpose of this PR is to enable rules, not to fix problems in old code.
The step that is turned off here is just checking for changes to the code. Since any new code is checked under stricter rules.

equalf
errf
etcd*
etcdserver
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks unnecessary as previous entry matches it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it doesn't work that way.
XXX* can be joined only with *YYY patterns.

tlsdialer
tmpl
tnt+
tntp
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks unnecessary as previous entry matches it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See, comment about etcd*

@@ -145,7 +145,7 @@ func startEtcd(t *testing.T, opts etcdOpts) integration.LazyCluster {
func etcdPut(t *testing.T, etcd *clientv3.Client, key, value string) {
t.Helper()
var (
presp *clientv3.PutResponse
presp *clientv3.PutResponse // spell-checker:ignore presp
Copy link
Contributor

Choose a reason for hiding this comment

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

In 'cli/cluster/integration_test.go' the same identifier changed to pResp. Please consider single approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pResp

@@ -273,10 +273,10 @@ func TestConnectEtcd(t *testing.T) {
etcdPut(t, etcd, "foo", "bar")

ctx, cancel := context.WithTimeout(context.Background(), timeout)
gresp, err := etcd.Get(ctx, "foo")
resp, err := etcd.Get(ctx, "foo")
Copy link
Contributor

Choose a reason for hiding this comment

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

In 'cli/cluster/integration_test.go' the similar identifier presp changed to pResp. Please consider similar approach here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What will the letter p in this variable mean?
I don't see the need for it in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the similar approach I mean that if presp is changed to pResp then gresp should be changed to gResp (AFAIU pResp stands for PUT response and gResp stands for GET response).

@@ -502,6 +502,7 @@ func (publisher EtcdAllDataPublisher) Publish(revision int64, data []byte) error
txn = txn.If(cmps...)
}

// spell-checker:ignore tresp
Copy link
Contributor

Choose a reason for hiding this comment

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

In 'cli/cluster/integration_test.go' the similar identifier presp changed to pResp. Please consider similar approach here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tResp

@@ -155,7 +155,7 @@ def test_promote_cconfig_failovers(
"manual-failover-2",
"election-failover-1",
"election-failover-2",
"eleciton-failover-3",
"eleciton-failover-3", # spell-checker:disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"eleciton-failover-3", # spell-checker:disable-line
"election-failover-3",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the test will fail in this case.
The purpose of this PR is not to fix problematic code, but to enable spell checking.

@@ -44,7 +46,7 @@ def skip_no_helpers(request: pytest.FixtureRequest, completion: Completion) -> N


def pytest_configure(config):
config.addinivalue_line(
config.addinivalue_line( # cSpell:words addinivalue
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's possible I'd suggest to introduce separate project-words file for integration tests directory. All pytest identifiers that are currently in use could be listed there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My opinion is that lists of “bad” words are workaround, because they should not exist. The fact that now a list of words is made at the project level is because there are too many such words already found throughout the old code. Ideally, the names should immediately meet the requirements of the checker.

And such marks raise a “red” flag - that something needs to be done here with refactoring. The word list on other hand hides this problem.

@dmyger dmyger force-pushed the dmyger/tntp-3105_turn_on_spell_checker_for_code branch from 20a96d7 to 707afff Compare July 22, 2025 10:17
@dmyger dmyger force-pushed the dmyger/tntp-3105_turn_on_spell_checker_for_code branch from 707afff to 25a4d6e Compare July 23, 2025 17:03
@dmyger dmyger requested a review from elhimov July 23, 2025 17:32
dmyger added 3 commits July 23, 2025 20:35
Part of #TNTP-3105
Due to formatting changes, it triggers the linter's errors.
FIXME: turn on the checking with the following PR.

Part of #TNTP-3105
@dmyger dmyger force-pushed the dmyger/tntp-3105_turn_on_spell_checker_for_code branch from 25a4d6e to 89506dd Compare July 23, 2025 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-ci Enables full ci tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants