Skip to content

Conversation

@bhearsum
Copy link
Contributor

…ocessing any of them

This is a follow to address something @ahal called out in the initial review of landoscript.

@bhearsum bhearsum mentioned this pull request Apr 18, 2025
@bhearsum bhearsum requested a review from a team April 18, 2025 01:01
Comment on lines 246 to 245
# done here because setup_test sets up github_installation_response too soon...argh
from yarl import URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the comment meant to go with the removed line? (either way, seems unrelated with this patch?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment is about repeating all of the test set up overall (instead of using setup_test), but I can see how it is...not particularly great. I'll improve it.

The import change is unrelated, yeah. It's not harmful, but I'll revert it to keep things clean. (Another follow up I'm planning is to add isort, which will help clean some of these things up.)

@@ -1,6 +1,7 @@
import pytest
from scriptworker_client.github_client import TransportQueryError
from yarl import URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. The removal is removing an unused import, but as you say, unrelated.

@bhearsum bhearsum force-pushed the lando-toml-contents branch from 1a8711e to 18b4fea Compare April 21, 2025 13:15
@bhearsum bhearsum requested a review from jcristau April 21, 2025 13:22
@bhearsum bhearsum force-pushed the lando-toml-contents branch from 18b4fea to 7ec66bb Compare April 22, 2025 12:27
…ocessing any of them

This is a follow to address something [@ahal called out in the initial review of landoscript](mozilla-releng#1135 (comment)).
@bhearsum bhearsum force-pushed the lando-toml-contents branch from 7ec66bb to a5877b1 Compare April 22, 2025 12:30
@bhearsum bhearsum merged commit 7615188 into mozilla-releng:master Apr 22, 2025
9 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