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

Some recommendations when raising warnings and exceptions #59

Open
framunoz opened this issue Jan 2, 2025 · 0 comments
Open

Some recommendations when raising warnings and exceptions #59

framunoz opened this issue Jan 2, 2025 · 0 comments

Comments

@framunoz
Copy link

framunoz commented Jan 2, 2025

I have noticed that in several modules warnings and exceptions are being thrown in a very general way. For example:

  • In the module ee_extra/QA/clouds.py you can find the following code:
    platformDict = _get_platform_STAC(x)

    if platformDict["platform"] not in list(lookup.keys()):
        warnings.warn("This platform is not supported for cloud masking.")
        return x

It could be changed to:

    platformDict = _get_platform_STAC(x)
    platform = platformDict["platform"]

    if platform not in list(lookup.keys()):
        warnings.warn(f"Platform {platform} is not supported for cloud masking.")
        return x

To make the message more descriptive. Moreover, all the code could be refactored by including the platform variable.

  • In the ee_extra/STAC/utils.py module, if the args argument does not have a "system:id", then it would throw the exception at the end, but that would be very confusing to the client using this functionality. Also, throwing a generic Exception is not the best way to handle exceptions.

That said, for this module, I propose the following implementation:

import ...  # etc...

# New custom exceptions
class ElementWithoutIdException(Exception):
    """Raised when an element does not have an ID."""

    def __init__(self, elem):
        super().__init__("Element does not have an 'system:id'.")
        self.elem = elem

class PlatformNotSupportedError(Exception):
    """Raised when a platform is not supported."""

    def __init__(self, platform: str):
        super().__init__(f"Platform '{platform}' not supported.")
        self.platform = platform

def _get_platform_STAC(args: Union[ee.Image, ee.ImageCollection]) -> dict:
    ...

    # Line 25
    ID = args.get("system:id").getInfo()
    if ID is None:
        raise ElementWithoutIdException(args)

    ...

    # line 50
    if plt is None:
        raise PlatformNotSupportedError(plt)

    ...

These custom exceptions have the advantage of also including the elements that caused the exception to be thrown for use in exception handling. This code exemplifies this:

class CustomException(Exception):
    def __init__(self, arg):
        super().__init__(f"Look at this arg!: '{arg}'")
        self.arg = arg

try:
    raise CustomException("A dummy arg")
except CustomException as e:
    print(f"Now a know the arg: '{e.arg}'")
>>> Now a know the arg: 'A dummy arg'

If you wish, I can open a PR to include these new changes, and try to find some other cases that are out there. Have a happy day!

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

No branches or pull requests

1 participant