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 Ruff CI errors #938

Merged
merged 3 commits into from
Feb 2, 2024
Merged

Conversation

stared
Copy link
Contributor

@stared stared commented Jan 15, 2024

A potential solution for #937

Do you give it a go @Serene-Arc ?
I am open to other solutions for Ruff, if needed.

@stared stared changed the base branch from master to development January 15, 2024 14:19
@stared stared changed the title Ruff-command-fix test Fix Ruff CI errors Jan 15, 2024
@Serene-Arc
Copy link
Owner

Hi, there are a bunch of other changes here that don't relate to the ruff error. Can you give your reasoning for those or maybe put them in their own explanatory commits?

@Serene-Arc
Copy link
Owner

I'm honestly at a bit of a loss to explain why the master protection check is failing. That's weird.

@stared
Copy link
Contributor Author

stared commented Jan 16, 2024

Hi @Serene-Arc !

These changes are to fix linting errors discovered by Ruff (previously, it was not seeing that, as it was not working):

ruff check .
tests/integration_tests/test_download_integration.py:44:9: PT014 Duplicate of test case at index 1 in `@pytest_mark.parametrize`
   |
42 |         ["-s", "EmpireDidNothingWrong", "-L", 3],
43 |         ["-s", "r/EmpireDidNothingWrong", "-L", 3],
44 |         ["-s", "r/EmpireDidNothingWrong", "-L", 3],
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT014
45 |         ["-s", "EmpireDidNothingWrong", "-L", 3],
46 |         ["-s", "https://www.reddit.com/r/TrollXChromosomes/", "-L", 3],
   |
   = help: Remove duplicate test case

tests/integration_tests/test_download_integration.py:45:9: PT014 Duplicate of test case at index 0 in `@pytest_mark.parametrize`
   |
43 |         ["-s", "r/EmpireDidNothingWrong", "-L", 3],
44 |         ["-s", "r/EmpireDidNothingWrong", "-L", 3],
45 |         ["-s", "EmpireDidNothingWrong", "-L", 3],
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT014
46 |         ["-s", "https://www.reddit.com/r/TrollXChromosomes/", "-L", 3],
47 |         ["-s", "r/TrollXChromosomes/", "-L", 3],
   |
   = help: Remove duplicate test case

tests/test_file_name_formatter.py:534:85: PTH201 [*] Do not pass the current directory explicitly to `Path`
    |
532 |     test_filename: str, test_ending: str, expected_end: str, test_formatter: FileNameFormatter
533 | ):
534 |     result = test_formatter.limit_file_name_length(test_filename, test_ending, Path("."))
    |                                                                                     ^^^ PTH201
535 |     assert result.name.endswith(expected_end)
536 |     assert len(str(result)) <= FileNameFormatter.find_max_path_length()
    |
    = help: Remove the current directory argument

Found 3 errors.
[*] 1 fixable with the `--fix` option (2 hidden fixes can be enabled with the `--unsafe-fixes` option).

I committed these changes with a separate commit (ff27049), for clarity.

When it comes to the "Protect master branch / merge_check (pull_request)" - it is protected (and from one I see, unused). By mistake, I created a PR to master, only later changed it to development (but it didn't rerun tests).

Sadly, "Python Test / test (ubuntu-latest, 3.9, .sh) (pull_request)" is failing, but it was in the same way with all other PRs and commits. I am unsure if it was due to code changes or Reddit changing its way - it deserves another Issue/PR. Tests with external API calls are (while often necessary) a tricky thing.

@Serene-Arc
Copy link
Owner

The tests fail due to the token that Ali has set up as the repository secret still being the BDFR's token, instead of a private one. That means that we're sharing the rate limit with every other BDFR user that hasn't changed the token, and the tests fail. I don't have the ability to change that token, so the tests are failing.

@stared
Copy link
Contributor Author

stared commented Jan 19, 2024

Too bad! I assume Ali does not have space to change that.

In this case, it is burning 3x40min of GitHub Actions, and hardly informative. For one project, I did two test sets - one internal and the other - everything involving an external API.

Anyway, regarding Ruff - does it look good to you, and is it up for merging?

@stared
Copy link
Contributor Author

stared commented Feb 1, 2024

@Serene-Arc And how about this PR? Would you like to merge it?

@Serene-Arc
Copy link
Owner

lgtm

@Serene-Arc Serene-Arc merged commit 257d444 into Serene-Arc:development Feb 2, 2024
1 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants