-
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 10 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. Just changed 2 comments for line length. |
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 |
---|---|---|
|
@@ -7,7 +7,6 @@ | |
|
||
""" | ||
|
||
from os.path import basename | ||
from pathlib import Path | ||
from shutil import rmtree | ||
from tempfile import mkdtemp | ||
|
@@ -73,7 +72,8 @@ def get_asset_from_item(self, item: Item) -> Asset: | |
This is used to select which asset is used by HyBIG to generate | ||
the browse image following these steps: | ||
|
||
1. If found, return the first asset with 'visual' in any of the item's values' roles. | ||
1. If found, return the first asset with 'visual' in any of the item's | ||
values' roles. | ||
2. If found, return the first asset that has 'data' in its item's values' roles. | ||
3. Raise a StopIteration error. | ||
|
||
|
@@ -145,7 +145,7 @@ def process_item(self, item: Item, source: HarmonySource) -> Item: | |
|
||
except Exception as exception: | ||
self.logger.exception(exception) | ||
raise HyBIGServiceError from exception | ||
raise HyBIGServiceError(str(exception)) from exception | ||
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. This actually is a change before we logged the correct exception and raised a new one from the old, but the workflow-ui didn't show any information about it. |
||
finally: | ||
rmtree(working_directory) | ||
|
||
|
@@ -174,7 +174,7 @@ def create_output_stac_item( | |
"""Create an output STAC item used to access the browse imagery and | ||
ESRI world file as staged in S3. | ||
|
||
asset_items are an array of tuples where the tuples should be: (name, | ||
item_assets is an array of tuples where the tuples should be: (name, | ||
url, role) | ||
|
||
""" | ||
|
@@ -191,7 +191,7 @@ def create_output_stac_item( | |
|
||
output_stac_item.assets[asset_name] = Asset( | ||
url, | ||
title=basename(url), | ||
title=Path(url).name, | ||
media_type=get_file_mime_type(url), | ||
roles=[role], | ||
) | ||
|
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]}' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
coverage~=7.6.0 | ||
pycodestyle~=2.12.0 | ||
pylint~=3.2.6 | ||
pytest~=8.3.1 | ||
unittest-xml-reporting~=3.2.0 | ||
pylint~=3.3.1 | ||
pytest-cov~=6.0.0 | ||
pytest~=8.3.3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,28 +20,19 @@ STATUS=0 | |
|
||
export HDF5_DISABLE_VERSION_CHECK=1 | ||
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. This line doesn't seem necessary. I wonder if it's here from another project... 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. Yeah, this is some straight-up cargo cult stuff from some older version of this script which has been copied and pasted from project to project. Agree with cutting it. (One day this file will be a glorious one liner that just becomes part of the CI/CD directly) |
||
|
||
# Run the standard set of unit tests, producing JUnit compatible output | ||
coverage run -m pytest tests --junitxml=tests/reports/test-results-"$(date +'%Y%m%d%H%M%S')".xml | ||
RESULT=$? | ||
|
||
if [ "$RESULT" -ne "0" ]; then | ||
STATUS=1 | ||
echo "ERROR: unittest generated errors" | ||
fi | ||
# Exit status used to report back to caller | ||
STATUS=0 | ||
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. Looking at the raw file Double declaration of STATUS=0 on Line 19 Line 25 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. |
||
|
||
echo "\n" | ||
echo "Test Coverage Estimates" | ||
coverage report --omit="tests/*" | ||
coverage html --omit="tests/*" -d tests/coverage | ||
# Run the standard set of unit tests, producing JUnit compatible output | ||
pytest --cov=hybig --cov=harmony_service \ | ||
--cov-report=html:reports/coverage \ | ||
--cov-report term \ | ||
--junitxml=reports/test-reports/test-results-"$(date +'%Y%m%d%H%M%S')".xml || STATUS=1 | ||
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. Not sure if STATUS=1 is set from code above. 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. This is where we set it when the test fails so that the exit code is non-zero on failures. |
||
|
||
# Run pylint | ||
# Ignored errors/warnings: | ||
# W1203 - use of f-strings in log statements. This warning is leftover from | ||
# using ''.format() vs % notation. For more information, see: | ||
# https://github.com/PyCQA/pylint/issues/2354#issuecomment-414526879 | ||
pylint hybig harmony_service --disable=W1203 | ||
RESULT=$? | ||
RESULT=$((3 & $RESULT)) | ||
pylint hybig harmony_service | ||
RESULT=$((3 & $?)) | ||
|
||
if [ "$RESULT" -ne "0" ]; then | ||
STATUS=1 | ||
|
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. 👍 |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
"""Module definition for test_service.""" |
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!