-
Notifications
You must be signed in to change notification settings - Fork 5
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
DAS-NONE: Random small fixes. #38
Changes from all commits
ea94925
b1cf9e8
8304a9c
192f7e6
d094785
c051954
b6b05f7
920ab4d
b9bd5c5
edf22bf
8bf518b
38d9f1b
54049a8
4fcded7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ | |
# and updates the entrypoint of the new service container | ||
# | ||
############################################################################### | ||
FROM python:3.11 | ||
FROM python:3.12 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice - I'm tempted to suggest releasing a version of HyBIG with just this change of Python version. Feels like it could be a fair candidate for a patch version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really don't want to because it's a bunch of overhead for something that will be replaced as soon as I finish all that overhead. The next release will change functionality significantly and require updated regression-tests. I could release it and not deploy it, but that also seems kind of dumb in that you've just created another version that is lying around. I just did this in a separate PR to make it easier for someone to review and test. Since this does run against the existing regressions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. Given there isn't a dedicated ticket specifically for this clean up, I can buy that. |
||
|
||
WORKDIR "/home" | ||
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of scope for this PR - but if you are cleaning things up, you could tweak the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that. Maybe in a follow-on PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh, mixing pytest fixtures and unittest patches. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow that was a dumb 🐰 🕳️ 4fcded7 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ def get_tiled_file_extension(file_name: Path) -> str: | |
generate the correct one to pass in. | ||
|
||
""" | ||
ext_pattern = r"(\.r\d+c\d+)?\.(png|jpg|pgw|jgw|txt)(.aux.xml)?" | ||
ext_pattern = r'(\.r\d+c\d+)?\.(png|jpg|pgw|jgw|txt)(.aux.xml)?' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why change double quote to single quote? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both of those things. I don't have a preference, but I couldn't convince Owen to go to Black style formatting with the double quotes. I'm not actually sure how all of these became double quotes in the past because we did have some style checks that I thought would have caught them. The new ruff-formatting added and used by pre-commit.ci, runs on all files, and found all of the places where I just accidentally left double quotes in. So tl;dr Yes, consistency and coding standards. ok, I looked, before we used black-jupyter with "skip-string-normalization" which just leaves strings however you put them in, and doesn't change them. the pyproject.toml specifies "single" for quote style to force you to choose one. (consistency) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
😅
Yup, this.
I think somewhere in PEP008 there's a sentence about whether to use single or double quotes. The sentiment is basically: it doesn't really matter which, but pick one and use it consistently. |
||
match = re.search(ext_pattern, file_name.name) | ||
return match.group() | ||
|
||
|
@@ -34,7 +34,7 @@ def get_asset_name(name: str, url: str) -> str: | |
dictionary. | ||
|
||
""" | ||
tiled_pattern = r"\.(r\d+c\d+)\." | ||
tiled_pattern = r'\.(r\d+c\d+)\.' | ||
tile_id = re.search(tiled_pattern, url) | ||
if tile_id is not None: | ||
name = f'{name}_{tile_id.groups()[0]}' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!