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

ci: Fix typos discovered by codespell #5778

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Sep 12, 2023

Related Issues

  • fixes #issue-number

Proposed Changes:

https://pypi.org/project/codespell

How did you test it?

Notes for the reviewer

Checklist

@silvanocerza
Copy link
Contributor

I have some doubts about this.

I like this cause typos might make it harder to search for some words. On the other hand I wouldn't want to start a war on which is the correct way of writing a word. e.g color or colour

Another HUGE issue that I see is that this is focused on english only and doesn't understand the context nor whether it's actually trying to fix english or another language. See test/nodes/test_ranker.py as an example. It's trying to fix real german words. 😅

Not really sure whether to accept this or not to be fair. 😕

@coveralls
Copy link
Collaborator

coveralls commented Sep 12, 2023

Pull Request Test Coverage Report for Build 6173376004

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 48.966%

Totals Coverage Status
Change from base Build 6173046187: 0.0%
Covered Lines: 11818
Relevant Lines: 24135

💛 - Coveralls

@cclauss
Copy link
Contributor Author

cclauss commented Sep 12, 2023

Skipped over the German content and would be easy to add colour to the ignores if it existed in this repo.

[tool.codespell]
ignore-words-list = "ans,astroid,nd,ned,nin,requeset,ue"
quiet-level = 3
skip = "./test/nodes/*,./test/others/*,./test/samples/*"

@dfokina
Copy link
Contributor

dfokina commented Sep 12, 2023

From what I see here, except for the German words, everything it corrected in English is good and helpful, to be honest.
Looks like it uses US English, too, so even if it corrects colour to color, it would be actually in accordance with our guidelines @silvanocerza 👌
So I am not opposed to accepting this 🙂

@silvanocerza
Copy link
Contributor

Cool for me then. 👍
I'll leave the final decision to you @dfokina.

@dfokina
Copy link
Contributor

dfokina commented Sep 12, 2023

Approved 👍

pyproject.toml Outdated
@@ -278,6 +278,10 @@ packages = [
line-length = 120
skip_magic_trailing_comma = true # For compatibility with pydoc>=4.6, check if still needed.

[tool.codespell]
ignore-words-list = "ans,astroid,nd,ned,nin,requeset,ue"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One lingering question... Was Requeset misspelled on purpose to make the assertion fail?

assert "Too Many Requeset" in caplog.text

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a typo copy and pasted, the actual log message is incorrect too.

"Failed to insert a batch of '%s' documents because of a 'Too Many Requeset' response. "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed both.

@dfokina
Copy link
Contributor

dfokina commented Sep 12, 2023

Also, some release notes are needed before merging this, I believe: https://github.com/deepset-ai/haystack/blob/main/CONTRIBUTING.md#release-notes

@ZanSara ZanSara removed their request for review September 12, 2023 16:25
@masci masci added the ignore-for-release-notes PRs with this flag won't be included in the release notes. label Sep 13, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this in my opinion. I already marked the PR with the ignore-for-release-notes label.

@silvanocerza
Copy link
Contributor

silvanocerza commented Sep 13, 2023

Linting is failing. Seems quite strange. 🤔

https://github.com/deepset-ai/haystack/actions/runs/6162226091/job/16723238817?pr=5778

I guess those changes have been done before introducing linting checks. We should fix them in a separate PR I think.

On a side note I fixed the license compliance workflow so it won't show as failing in PRs from forks. See #5791.

@cclauss
Copy link
Contributor Author

cclauss commented Sep 13, 2023

Lint failures are fixed in #5783

I will remove reno entry despite #5778 (comment) and

@silvanocerza
Copy link
Contributor

Merged in main to get linting and license fixes.

@silvanocerza
Copy link
Contributor

Let's goooo! 🚀

Thanks again! 🙏

@silvanocerza silvanocerza merged commit 6dd52d9 into deepset-ai:main Sep 13, 2023
77 checks passed
@cclauss cclauss deleted the codepsell branch September 13, 2023 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants