Add codespell (config, workflow) and use it to fix found typos (and some manully) #8173

yarikoptic · 2024-04-09T16:24:52Z

Proposed fixes:

Make ckan typo free. I had to change some variable names in some tests IIRC but overall I think there should be no effect on functionality.

Features:

includes tests covering changes
includes updated documentation
includes user-visible changes
includes API changes
includes bugfix for possible backport

Please [X] all the boxes above that apply

yarikoptic · 2024-04-09T18:31:12Z

ok, I fixed up testing by allowing for grup. Hopefully all good now. Enjoy typo free ckan, feel welcome to tune PR to your liking

EricSoroos

I've flagged all the locations where there's a potential behavior change -- we need to either validate that this doesn't change the behavior or that it doesn't cause regressions.

EricSoroos · 2024-04-11T13:51:06Z

ckan/lib/dictization/__init__.py

-    for contraint in table.constraints:
-        if isinstance(contraint, sqlalchemy.UniqueConstraint):
-            columns = [column.name for column in contraint.columns]
+    for constraint in table.constraints:


local variable change -- looks safe

EricSoroos · 2024-04-11T13:53:48Z

ckan/public/base/javascript/modules/table-toggle-more.js

@@ -35,8 +35,8 @@ this.ckan.module('table-toggle-more', function($) {
          '</tr>'
        ].join('\n');

-       var seperator = $(template_seperator).insertAfter($('.toggle-more:last-child', this.el));
-        $(template_more).insertAfter(seperator);
+       var separator = $(template_seperator).insertAfter($('.toggle-more:last-child', this.el));


Looks safe, but missed template_seperator

nice eye! ;-) moreover that one is present in css and scss. I will fix all of them (current commit c79bade if would not be rebased)

rewritten commit 72aaf40

EricSoroos · 2024-04-11T13:55:54Z

ckan/templates/package/snippets/stages.html

@@ -1,9 +1,9 @@
 {#
 Inserts a stepped progress indicator for the new package form. Each stage can
-have one of three states, "uncomplete", "complete" and "active".
+have one of three states, "incomplete", "complete" and "active".


This make the documentation incorrect: (lines 16-20 in this file)

{% set s2 = stages[1] or 'uncomplete' %} {% set dataset_type = dataset_type or h.default_package_type() %} {% if s1 != 'uncomplete' %}{% set class = 'stage-1' %}{% endif %} {% if s2 != 'uncomplete' %}{% set class = 'stage-2' %}{% endif %}

Replaced all of them in 3454637 - no more "uncomplete"s should be found!

rewritten commit is 51bfe61

EricSoroos · 2024-04-11T13:57:28Z

ckan/tests/pytest_ckan/fixtures.py

@@ -474,7 +474,7 @@ def test_example():

 class FakeFileStorage(FlaskFileStorage):
    def __init__(self, stream: IO[bytes], filename: str):
-        super(FakeFileStorage, self).__init__(stream, filename, "uplod")
+        super(FakeFileStorage, self).__init__(stream, filename, "upload")


Is this relied on anywhere?

wouldn't it lead to some failed tests? seems to be green now.

EricSoroos · 2024-04-11T13:58:10Z

ckanext/activity/templates/snippets/activities/changed_resource.html

@@ -6,7 +6,7 @@
  {{ _('{actor} updated the resource {resource} in the dataset {dataset}').format(
    actor=ah.actor(activity),
    resource=ah.resource(activity),
-    dataset=ah.datset(activity)
+    dataset=ah.dataset(activity)


This might be an existing bug.

EricSoroos · 2024-04-11T13:59:23Z

ckanext/activity/tests/test_views.py

@@ -51,7 +51,7 @@ def test_simple(self, app):
    def test_simple_for_custom_org_type(self, app):
        """Checking the template shows the activity stream."""
        user = factories.User()
-        org = factories.Organization(user=user, type="grup")
+        org = factories.Organization(user=user, type="group")


This could be intentional.

ok -- I will reorder commits so I skip grup before I run thorough fixup. It should leave it behind... did that and diff showed indeed only this

❯ git diff enh-codespell.. diff --git a/ckanext/activity/tests/test_views.py b/ckanext/activity/tests/test_views.py index c7bf87a46..ce2e7584f 100644 --- a/ckanext/activity/tests/test_views.py +++ b/ckanext/activity/tests/test_views.py @@ -51,7 +51,7 @@ class TestOrganization(object): def test_simple_for_custom_org_type(self, app): """Checking the template shows the activity stream.""" user = factories.User() - org = factories.Organization(user=user, type="group") + org = factories.Organization(user=user, type="grup") url = url_for("activity.organization_activity", id=org["id"]) response = app.get(url)

EricSoroos · 2024-04-11T14:00:15Z

ckanext/example_itranslation/tests/test_plugin.py

@@ -27,7 +27,7 @@ def test_translated_string_in_core_templates(self, app):
            url=plugins.toolkit.url_for(u"home.index", locale="fr"),
        )
        assert helpers.body_contains(response, "Overwritten string in ckan.mo")
-        assert not helpers.body_contains(response, "Connexion")
+        assert not helpers.body_contains(response, "Connection")


This could be changing the test

rright... and talks about "fr" for which it is correct I guess:

❯ git grep -a Connexion ckan/i18n/fr/LC_MESSAGES/ckan.mo:ckan/i18n/fr/LC_MESSAGES/ckan.po:msgstr "Connexion" ❯ git grep Connexion Binary file ckan/i18n/fr/LC_MESSAGES/ckan.mo matches ckan/i18n/fr/LC_MESSAGES/ckan.po:msgstr "Connexion"

I will need to add pragma to skip this line and rerun "fixups"...

EricSoroos · 2024-04-15T07:55:00Z

It was mentioned on the dev call that depending on how we're feeling about adding code to the actions and the security implications -- we may just want to accept the spelling changes and deal with the ongoing maintenance in a separate PR.

yarikoptic · 2024-04-15T22:23:19Z

up to you -- I can easily pull out the workflow if so desired. Note that it has

permissions:
  contents: read

and no secrets explicitly exposed -- should be safe IMHO (shouldn't push etc). I guess there could still be a malicious vector of attack -- me, since IIRC next time "I" [*] submit a PR against this workflow (may be adding some secrets leaking?) -- they could be exposed? Or what other vector do you foresee?

[*] "I" could be an attacker using my stolen account?

…ut ignoring the failure due to ambigous typos === Do not change lines below === { "chain": [ "3e69bc9d304d2a3ab16238c591c9a85e65d152aa", "e71e761523037c3d9b505fb42fbcf3be3a7118e4" ], "cmd": "codespell -w || :", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

=== Do not change lines below === { "chain": [ "e96808eedb54d367087e495c7c0d279cca32362e" ], "cmd": "codespell -w -i 3 -C 2", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

…css, scss, js === Do not change lines below === { "chain": [], "cmd": "git-sedi seperator separator", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

…' -- also in css, scss, js === Do not change lines below === { "chain": [], "cmd": "git-sedi uncomplete incomplete", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

yarikoptic · 2024-04-15T22:55:18Z

Thank you @EricSoroos for the review!

ok, pushed rebase with additional fixes and reran of the codespell to not affect mentioned above grup and Connexion, while fixing more of detected typos

Overall diff from old version to current one (although also available in github interface in "Compare")

diff --git a/ckan/public/base/css/main-rtl.css b/ckan/public/base/css/main-rtl.css
index fb7ef9656..4ba76877d 100644
--- a/ckan/public/base/css/main-rtl.css
+++ b/ckan/public/base/css/main-rtl.css
@@ -13642,13 +13642,13 @@ td.diff_header {
 .js .table-toggle-more .show-less {
   display: none;
 }
-.js .table-toggle-more .toggle-seperator {
+.js .table-toggle-more .toggle-separator {
   display: table-row;
 }
-.js .table-toggle-more .toggle-seperator td {
+.js .table-toggle-more .toggle-separator td {
   height: 11px;
   padding: 0;
-  background-image: url("../../../base/images/table-seperator.png");
+  background-image: url("../../../base/images/table-separator.png");
 }
 .js .table .toggle-show td {
   background: none;
@@ -13660,7 +13660,7 @@ td.diff_header {
 .js .table-toggle-less .show-more {
   display: none;
 }
-.js .table-toggle-less .toggle-seperator {
+.js .table-toggle-less .toggle-separator {
   display: none;
 }
 
diff --git a/ckan/public/base/css/main.css b/ckan/public/base/css/main.css
index c139ff91e..144d95503 100644
--- a/ckan/public/base/css/main.css
+++ b/ckan/public/base/css/main.css
@@ -13642,13 +13642,13 @@ td.diff_header {
 .js .table-toggle-more .show-less {
   display: none;
 }
-.js .table-toggle-more .toggle-seperator {
+.js .table-toggle-more .toggle-separator {
   display: table-row;
 }
-.js .table-toggle-more .toggle-seperator td {
+.js .table-toggle-more .toggle-separator td {
   height: 11px;
   padding: 0;
-  background-image: url("../../../base/images/table-seperator.png");
+  background-image: url("../../../base/images/table-separator.png");
 }
 .js .table .toggle-show td {
   background: none;
@@ -13660,7 +13660,7 @@ td.diff_header {
 .js .table-toggle-less .show-more {
   display: none;
 }
-.js .table-toggle-less .toggle-seperator {
+.js .table-toggle-less .toggle-separator {
   display: none;
 }
 
diff --git a/ckan/public/base/javascript/modules/table-toggle-more.js b/ckan/public/base/javascript/modules/table-toggle-more.js
index 90fd7302f..99e3b51ea 100644
--- a/ckan/public/base/javascript/modules/table-toggle-more.js
+++ b/ckan/public/base/javascript/modules/table-toggle-more.js
@@ -28,14 +28,14 @@ this.ckan.module('table-toggle-more', function($) {
           '</td>',
           '</tr>'
         ].join('\n');
-        var template_seperator = [
-          '<tr class="toggle-seperator">',
+        var template_separator = [
+          '<tr class="toggle-separator">',
           '<td colspan="'+cols+'">',
           '</td>',
           '</tr>'
         ].join('\n');
 
-       var separator = $(template_seperator).insertAfter($('.toggle-more:last-child', this.el));
+       var separator = $(template_separator).insertAfter($('.toggle-more:last-child', this.el));
         $(template_more).insertAfter(separator);
 
         $('.show-more', this.el).on('click', this._onShowMore);
diff --git a/ckan/public/base/scss/_tables.scss b/ckan/public/base/scss/_tables.scss
index 2ba5b5433..4a160be47 100644
--- a/ckan/public/base/scss/_tables.scss
+++ b/ckan/public/base/scss/_tables.scss
@@ -43,12 +43,12 @@
         .show-less {
             display: none;
         }
-        .toggle-seperator {
+        .toggle-separator {
             display: table-row;
             td {
                 height: 11px;
                 padding: 0;
-                background-image: url("#{$imagePath}/table-seperator.png");
+                background-image: url("#{$imagePath}/table-separator.png");
             }
         }
     }
@@ -63,7 +63,7 @@
         .show-more {
             display: none;
         }
-        .toggle-seperator {
+        .toggle-separator {
             display: none;
         }
     }
diff --git a/ckan/public/base/vendor/jquery.js b/ckan/public/base/vendor/jquery.js
index fc6c299b7..248b5ac16 100644
--- a/ckan/public/base/vendor/jquery.js
+++ b/ckan/public/base/vendor/jquery.js
@@ -3991,7 +3991,7 @@ jQuery.extend( {
 	when: function( singleValue ) {
 		var
 
-			// count of uncompleted subordinates
+			// count of incompleted subordinates
 			remaining = arguments.length,
 
 			// count of unprocessed arguments
diff --git a/ckan/templates/package/snippets/stages.html b/ckan/templates/package/snippets/stages.html
index 5ec0d024d..0dfe18f59 100644
--- a/ckan/templates/package/snippets/stages.html
+++ b/ckan/templates/package/snippets/stages.html
@@ -13,11 +13,11 @@ Example:
 
 #}
 {% set s1 = stages[0] or 'active' %}
-{% set s2 = stages[1] or 'uncomplete' %}
+{% set s2 = stages[1] or 'incomplete' %}
 {% set dataset_type = dataset_type or h.default_package_type() %}
 
-{% if s1 != 'uncomplete' %}{% set class = 'stage-1' %}{% endif %}
-{% if s2 != 'uncomplete' %}{% set class = 'stage-2' %}{% endif %}
+{% if s1 != 'incomplete' %}{% set class = 'stage-1' %}{% endif %}
+{% if s2 != 'incomplete' %}{% set class = 'stage-2' %}{% endif %}
 
 <ol class="stages {{ class }}">
   <li class="first {{ s1 }}">
diff --git a/ckanext/activity/tests/test_views.py b/ckanext/activity/tests/test_views.py
index c7bf87a46..ce2e7584f 100644
--- a/ckanext/activity/tests/test_views.py
+++ b/ckanext/activity/tests/test_views.py
@@ -51,7 +51,7 @@ class TestOrganization(object):
     def test_simple_for_custom_org_type(self, app):
         """Checking the template shows the activity stream."""
         user = factories.User()
-        org = factories.Organization(user=user, type="group")
+        org = factories.Organization(user=user, type="grup")
 
         url = url_for("activity.organization_activity", id=org["id"])
         response = app.get(url)
diff --git a/ckanext/example_itranslation/tests/test_plugin.py b/ckanext/example_itranslation/tests/test_plugin.py
index 01822c04a..8ce4a7ecd 100644
--- a/ckanext/example_itranslation/tests/test_plugin.py
+++ b/ckanext/example_itranslation/tests/test_plugin.py
@@ -27,7 +27,7 @@ class TestExampleITranslationPlugin(object):
             url=plugins.toolkit.url_for(u"home.index", locale="fr"),
         )
         assert helpers.body_contains(response, "Overwritten string in ckan.mo")
-        assert not helpers.body_contains(response, "Connection")
+        assert not helpers.body_contains(response, "Connexion")  # codespell-ignore
 
         # double check the untranslated strings
         response = app.get(url=plugins.toolkit.url_for(u"home.index"),)

=== Do not change lines below === { "chain": [], "cmd": "git mv ./ckan/public/base/images/table-seperator.png ./ckan/public/base/images/table-separator.png", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

yarikoptic · 2024-04-16T00:52:46Z

ckan/public/base/css/main-rtl.css

  height: 11px;
  padding: 0;
-  background-image: url("../../../base/images/table-seperator.png");
+  background-image: url("../../../base/images/table-separator.png");


just now mentioned this. Pushed separate ea3c259 to rename the file as well.

ckan/public/base/vendor/jquery.js

yarikoptic · 2024-04-16T02:00:29Z

interesting, test failed but a bit surprising - no changes in related test, and test body looks like it should have better passed:

    def test_search_page_results(self, app):
        """Searching for datasets returns expected results."""
    
        factories.Dataset(name="dataset-one", title="Dataset One")
        factories.Dataset(name="dataset-two", title="Dataset Two")
        factories.Dataset(name="dataset-three", title="Dataset Three")
    
        search_url = url_for("dataset.search")
        search_results = app.get(search_url, query_string={'q': 'One'})
    
>       assert "1 dataset found" in search_results
E       AssertionError: assert '1 dataset found' in <WrapperTestResponse 16567 bytes [200 OK]>

a flaky test? I will accept my suggestion in #8173 (comment) and thus trigger rerun...

yarikoptic · 2024-04-16T12:35:14Z

indeed a known issue - #6537 . FWIW in recent con/tinuous downloaded logs for the past week this one was the only failure:

❯ git grep test_search_page_results | grep -v PASS
16/push/pull%2f8173/ea3c259/circleci-build_and_test-6604-failed/test-python-3/104-0.txt:ckan/tests/controllers/test_package.py::TestSearch::test_search_page_results FAILED [ 16%]
16/push/pull%2f8173/ea3c259/circleci-build_and_test-6604-failed/test-python-3/104-0.txt:_____________________ TestSearch.test_search_page_results ______________________
16/push/pull%2f8173/ea3c259/circleci-build_and_test-6604-failed/test-python-3/104-0.txt:    def test_search_page_results(self, app):
16/push/pull%2f8173/ea3c259/circleci-build_and_test-6604-failed/test-python-3/104-0.txt:FAILED ckan/tests/controllers/test_package.py::TestSearch::test_search_page_results - AssertionError: assert '1 dataset found' in <WrapperTestResponse 16567 bytes [200 OK]>

here is tinuous configuration I have used for `tinuous fetch`.. seems support for circle-ci could still be improved

repo: ckan/ckan
vars:
  path_prefix: '{year}//{month}//{day}/{type}/{type_id}/{commit[:7]}/{ci}-'
  common_path: '{path_prefix}{wf_name}-{number}-{common_status}/'
ci:
  github:
    paths:
      logs: '{common_path}'
      artifacts: '{common_path}'
      releases: 'releases/{release_tag}'
  circleci:
    paths:
      logs: '{common_path}{job_name}/{step}-{index}.txt'
since: 2024-04-08T00:00:00Z
types: [cron, pr, push, manual]
datalad:
  enabled: true
  cfg_proc: text2git

yarikoptic added 4 commits April 8, 2024 09:50

Add github action to codespell master on push and PRs

a456552

Add codespell config

67beb7a

add pragma skip

22648b1

inline skip a test query string

f5b6568

yarikoptic force-pushed the enh-codespell branch from bc347bb to 526c8f3 Compare April 9, 2024 18:06

wardi assigned EricSoroos Apr 11, 2024

EricSoroos reviewed Apr 15, 2024

View reviewed changes

yarikoptic added 8 commits April 15, 2024 18:36

Anotate codespell-ignore cases

e0a4dda

Skip grup and .egg-info folder

f12a306

Inline ignore Connexion in a test

5c27f2e

Fix under/over-dashing of the section where typo lead to change in width

fa64802

[DATALAD RUNCMD] Replace all 'seperator' with 'separator' -- also in …

72aaf40

…css, scss, js === Do not change lines below === { "chain": [], "cmd": "git-sedi seperator separator", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^

yarikoptic force-pushed the enh-codespell branch from 526c8f3 to 51bfe61 Compare April 15, 2024 22:52

yarikoptic commented Apr 16, 2024

View reviewed changes

ckan/public/base/vendor/jquery.js Outdated Show resolved Hide resolved

Minor comment fixup in ckan/public/base/vendor/jquery.js

fdf347b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add codespell (config, workflow) and use it to fix found typos (and some manully) #8173

Add codespell (config, workflow) and use it to fix found typos (and some manully) #8173

yarikoptic commented Apr 9, 2024

yarikoptic commented Apr 9, 2024

EricSoroos left a comment

EricSoroos Apr 11, 2024

EricSoroos Apr 11, 2024

yarikoptic Apr 15, 2024

yarikoptic Apr 15, 2024

EricSoroos Apr 11, 2024

yarikoptic Apr 15, 2024

yarikoptic Apr 15, 2024

EricSoroos Apr 11, 2024

yarikoptic Apr 15, 2024

EricSoroos Apr 11, 2024

EricSoroos Apr 11, 2024

yarikoptic Apr 15, 2024

EricSoroos Apr 11, 2024

yarikoptic Apr 15, 2024

EricSoroos commented Apr 15, 2024

yarikoptic commented Apr 15, 2024

yarikoptic commented Apr 15, 2024

yarikoptic Apr 16, 2024

yarikoptic commented Apr 16, 2024

yarikoptic commented Apr 16, 2024

Add codespell (config, workflow) and use it to fix found typos (and some manully) #8173

Are you sure you want to change the base?

Add codespell (config, workflow) and use it to fix found typos (and some manully) #8173

Conversation

yarikoptic commented Apr 9, 2024

Proposed fixes:

Features:

yarikoptic commented Apr 9, 2024

EricSoroos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricSoroos commented Apr 15, 2024

yarikoptic commented Apr 15, 2024

yarikoptic commented Apr 15, 2024

Choose a reason for hiding this comment

yarikoptic commented Apr 16, 2024

yarikoptic commented Apr 16, 2024