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

chore: Spelling #19648

Closed
wants to merge 106 commits into from
Closed

chore: Spelling #19648

wants to merge 106 commits into from

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Apr 11, 2022

SUMMARY

This PR corrects misspellings identified by the check-spelling action.

The misspellings have been reported at jsoref@d28d76e#commitcomment-70995512

The action reports that the changes in this PR would make it happy: jsoref@e50b893

Note: this PR does not include the action. If you're interested in running a spell check on every PR and push, that can be offered separately.

I'm aware that this PR is quite large, it also includes a couple of API changes (which probably won't be acceptable). I'll try to tag items

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Copy link
Contributor Author

@jsoref jsoref left a comment

Choose a reason for hiding this comment

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

The majority of the corrections here were automatically suggested by Google Sheets.

A number of changes were done by me manually.

All file renames (or failures to rename files) are on me. I tried to at least rename all the files needed for CI (but clearly missed at least one image file).

I'm quite happy to split PRs like this into smaller bits (e.g. by directory or file type). I try very hard to avoid squashing until very late in the process as it makes rebasing very risky (I already rebased this once and hit one conflict).

I'll probably do one push to fix the items I've noted in this review.

I may also be a bit sporadically available for the next few weeks, but I should be able to respond w/in a week (even if I can't perform an update).

CODE_OF_CONDUCT.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
Comment on lines 664 to 693
In terms of best practices please advoid blanket disablement of Pylint messages globally (via `.pylintrc`) or top-level within the file header, albeit there being a few exceptions. Disablement should occur inline as it prevents masking issues and provides context as to why said message is disabled.
In terms of best practices please avoid blanket disablement of Pylint messages globally (via `.pylintrc`) or top-level within the file header, albeit there being a few exceptions. Disablement should occur inline as it prevents masking issues and provides context as to why said message is disabled.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this PR is generally limited to spelling (including duplicate words). Grammatically, disabling would be better than disablement.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed

# Install dependencies to fix `curl https support error` and `elaying package configuration warning`
# Install dependencies to fix `curl https support error` and `delaying package configuration warning`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I saw this item flagged I guessed relaying, I was wrong.

Comment on lines 27 to 30
from github import BadCredentialsException, Github, PullRequest, Repository
from github import BadCredentialsException, GitHub, PullRequest, Repository
except ModuleNotFoundError:
print("PyGithub is a required package for this script")
print("PyGitHub is a required package for this script")
exit(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If these are third party modules, then these changes will break. It's trivial for me to drop individual change families although I'll probably try to carefully tease out the text and noncode instances of GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

can you check that this change actually works by installing pygithub and trying to import that class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested.

Apparently the project does at least occasionally call itself PyGitHub:
https://github.com/PyGithub/PyGithub/blob/001970d4a828017f704f6744a5775b4207a6523c/README.md?plain=1#L1

But, the classes/exceptions/methods aren't exposed via the proper spelling, so I'll have to fix up this commit (dropping most of the Python bits, but keeping the change for line 29).


As an aside, I can't get pip install -r ... to work, it dies on my m1 trying to do stuff using numpy (amd64 stuff at that!!):

            numpy/core/src/multiarray/scalartypes.c.src:2967:65: error: too few arguments to function call, expected 2, have 1
                return _Py_HashDouble((double) PyArrayScalar_VAL(obj, Float));
                       ~~~~~~~~~~~~~~                                       ^

I've tried a couple of versions of numpy and that didn't help.

- [14261](https://github.com/apache/superset/pull/14261) feat(native-filters): Show/Hide filter bar by metdata ff (#14261) (@simcha90)
- [14261](https://github.com/apache/superset/pull/14261) feat(native-filters): Show/Hide filter bar by metadata ff (#14261) (@simcha90)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some projects don't like changing release notes. Let me know if this is such a project, it's trivial for me to ignore files/paths.

Comment on lines 335 to 370
CURSOR_DESCRIPION = 2
CURSOR_DESCRIPTION = 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

API change

superset/views/base_api.py Show resolved Hide resolved
superset/viz.py Outdated Show resolved Hide resolved
and a final one with ANNOTATION_COUNT childs
and a final one with ANNOTATION_COUNT children
Copy link
Contributor Author

Choose a reason for hiding this comment

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

?

payload_override={"engine": "sqllite"},
payload_override={"engine": "sqlite"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

?

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Gave it a quick pass, maybe it would be better to split this PR in docs, python backend, frontend for example

superset/views/base_api.py Show resolved Hide resolved
superset/viz.py Outdated Show resolved Hide resolved
@jsoref
Copy link
Contributor Author

jsoref commented Apr 13, 2022

So, I've split these into pieces. Note that a couple conflict w/ each-other. I don't have the energy to deal w/ that now, but they're fairly trivial to deal w/ later.

Once each of the pieces are in, I'll rerun my tooling and see how things look and update this PR to pick up the pieces.

@rusackas
Copy link
Member

@jsoref now that the other ones have merged (hooray and thank you!) do you want to rebase this one and keep going, or shall we close it?

@codecov
Copy link

codecov bot commented Feb 19, 2023

Codecov Report

Merging #19648 (68257bf) into master (a823033) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 68257bf differs from pull request most recent head 40ebfa5. Consider uploading reports for the commit 40ebfa5 to get more accurate results

@@           Coverage Diff           @@
##           master   #19648   +/-   ##
=======================================
  Coverage   67.71%   67.71%           
=======================================
  Files        1918     1918           
  Lines       74151    74132   -19     
  Branches     8052     8043    -9     
=======================================
- Hits        50212    50201   -11     
+ Misses      21886    21881    -5     
+ Partials     2053     2050    -3     
Flag Coverage Δ
hive 52.72% <77.77%> (ø)
mysql 78.53% <100.00%> (ø)
postgres 78.60% <100.00%> (ø)
presto 52.65% <77.77%> (ø)
python 82.44% <100.00%> (ø)
sqlite 77.11% <100.00%> (ø)
unit 52.62% <44.44%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ges/superset-ui-core/src/query/buildQueryObject.ts 100.00% <ø> (ø)
...packages/superset-ui-core/src/query/types/Query.ts 100.00% <ø> (ø)
...set-ui-core/src/ui-overrides/ExtensionsRegistry.ts 100.00% <ø> (ø)
...ins/legacy-plugin-chart-partition/src/Partition.js 0.00% <ø> (ø)
...gin-chart-echarts/src/Timeseries/transformProps.ts 58.33% <ø> (ø)
...tend/plugins/plugin-chart-table/src/TableChart.tsx 44.91% <ø> (ø)
...erset-frontend/src/SqlLab/components/App/index.jsx 82.60% <ø> (-0.73%) ⬇️
...d/src/SqlLab/components/QueryAutoRefresh/index.tsx 58.62% <ø> (ø)
...c/components/DeprecatedSelect/DeprecatedSelect.tsx 41.33% <ø> (ø)
...rontend/src/components/MetadataBar/MetadataBar.tsx 98.18% <ø> (-0.04%) ⬇️
... and 38 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor Author

@jsoref jsoref left a comment

Choose a reason for hiding this comment

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

This is a naive conceptual rebase -- I made check-spelling reasonably happy, but didn't review the changes before pushing here.

CONTRIBUTING.md Show resolved Hide resolved
RELEASING/changelog.py Outdated Show resolved Hide resolved
RELEASING/release-notes-2-0/changelog.md Outdated Show resolved Hide resolved
docs/docs/databases/sql-server.mdx Show resolved Hide resolved
@@ -16,7 +16,7 @@
# under the License.
"""update base metrics

Note that the metrics table was previously partially modifed by revision
Note that the metrics table was previously partially modified by revision
f231d82b9b26.

Revision ID: e9df189e5c7e
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment to note a renamed file below

superset/viz.py Show resolved Hide resolved
@rusackas
Copy link
Member

rusackas commented Apr 6, 2023

I think the bulk of these changes were handled in your more granular PRs (thanks again for those). Should we rebase this one, or scrap it?

Clearly, we should follow up when time allows and add your tooling to our CI pipeline! SOOO MANY TYPOS!

Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

@dpgaspar @villebro I'm not sure if there's any risk in renaming a migration file like this. Do you know?

Copy link
Member

Choose a reason for hiding this comment

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

Again, nervous about renaming these migrations... not sure if there's a real risk.

tests/integration_tests/core_tests.py Show resolved Hide resolved
jsoref added 16 commits April 7, 2023 13:32
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
@rusackas
Copy link
Member

rusackas commented Apr 7, 2023

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

@rusackas Ephemeral environment spinning up at http://54.191.2.216:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants