Skip to content

rename id in connection.load_collection #245

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+43 −15
Open

rename id in connection.load_collection #245

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0963f23
rename id in connection.load_collection
jonathom Oct 5, 2021
69bc5ce
collection_id deprecated in favor if id
jonathom Oct 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions openeo/rest/connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ def datacube_from_json(self, src: Union[str, Path], parameters: dict = None) ->

def load_collection(
self,
collection_id: str,
id: str,
spatial_extent: Optional[Dict[str, float]] = None,
temporal_extent: Optional[List[Union[str, datetime.datetime, datetime.date]]] = None,
bands: Optional[List[str]] = None,
Expand All @@ -833,7 +833,7 @@ def load_collection(
"""
Load a DataCube by collection id.

:param collection_id: image collection identifier
Copy link
Member

@m-mohr m-mohr Oct 5, 2021

Choose a reason for hiding this comment

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

Should this be deprecated instead of being removed?

Copy link
Member Author

@jonathom jonathom Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Would I just put @deprecated("Use 'id' instead") above the :param collection_id line?

Copy link
Member

@m-mohr m-mohr Oct 5, 2021

Choose a reason for hiding this comment

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

Sounds good to me, but @soxofaan should have the final vote.

Copy link
Member

@soxofaan soxofaan Oct 5, 2021

Choose a reason for hiding this comment

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

I don't think we can make a breaking change like this.
Existing code that uses keyword argument load_collection(collection_id="S2") will break.

There should be a fallback mechanism:

  • give id default value None
  • add a **kwargs and check if collection_id is set (and raise deprecation warning if that is the case)

Copy link
Member

@m-mohr m-mohr Oct 5, 2021

Choose a reason for hiding this comment

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

Yes, that's what I meant to say :-D

Copy link
Member Author

@jonathom jonathom Oct 5, 2021

Choose a reason for hiding this comment

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

@soxofaan

def load_collection(
    self,
    id: str = None,
[...]
    if "collection_id" in kwargs:
        id = kwargs["collection_id"]
        warnings.warn("The use of `collection_id` is deprecated, use `id` instead.", DeprecationWarning)

like so?

:param id: image collection identifier
:param spatial_extent: limit data to specified bounding box or polygons
:param temporal_extent: limit data to specified temporal interval
:param bands: only add the specified bands
Expand All @@ -842,13 +842,13 @@ def load_collection(
"""
if self._api_version.at_least("1.0.0"):
return DataCube.load_collection(
collection_id=collection_id, connection=self,
collection_id=id, connection=self,
Copy link
Member

@soxofaan soxofaan Oct 5, 2021

Choose a reason for hiding this comment

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

I think the load_collection methods of DataCube and ImageCollectionClient should also be addressed

but here it's fine to do it in a backwards incompatible way because these are practically "internal" methods nobody should be using

spatial_extent=spatial_extent, temporal_extent=temporal_extent, bands=bands, properties=properties,
fetch_metadata=fetch_metadata,
)
else:
return ImageCollectionClient.load_collection(
collection_id=collection_id, session=self,
collection_id=id, session=self,
spatial_extent=spatial_extent, temporal_extent=temporal_extent, bands=bands
)

Expand Down