Deprecation warning showing from internal call to DataGranule.size()

[julober]

This deprecation warning is being shown from an internal call to DataGranule.size(). The warning should only display for the user when they are calling that function.

[trey-stafford]

Introduced in #1267 and ultimately addressed by #1187

[asteiker]

@andypbarrett Is this warning expected just with a search_data() request? I thought this was specific to the methods that could be called from the results.

[trey-stafford]

Is this warning expected just with a search_data() request?

From a user perspective, no. The warning is raised because search_data() calls DataGranule.size() internally, raising the warning.

My initial thinking is we could either

• suppress warnings arising from internal use of depreciated methods
• Add the size property and use it internally, but also continue to expose size() as a method until v1, when it would be removed. This would allow users to migrate their calls before they break in v1.

[mfisher87]

Add the size property and use it internally, but also continue to expose size() as a method until v1, when it would be removed. This would allow users to migrate their calls before they break in v1.

I think we should do this, and IIRC think we're only missing the "use it internally" part!

IMO we should not do a release until we fix this.

@asteiker do we have a label to mark an issue as "blocking release"?

[chuckwondo]

Add the size property and use it internally, but also continue to expose size() as a method until v1, when it would be removed. This would allow users to migrate their calls before they break in v1.

I think we should do this, and IIRC think we're only missing the "use it internally" part!

It is not possible to have a property and a method of the same name. Technically, a property is a method (decorated with @property), so pedantically speaking, I suspect your suggestion is to have an attribute and a method of the same name, which is also not possible.

We would need to add a "private" property with a different name to avoid a name conflict. In this case, perhaps we can name the property (method with the @property decorator) _size to avoid a name conflict and internally reference the _size property.

Specifically, I suggest the following:

1. Add a new _size property (method decorated with @property)
2. Move all code after the warning in the size method over to the new _size property
3. After the warning in the size method, add the statement return self._size
4. Change all internal calls to the size method into references to the new _size property.

When we are ready to remove the size method:

1. Delete the size method
2. Rename the _size property to size (with the aid of a capable IDE, this rename can also automatically update all internal references)

Of course, all of these steps apply to all of the other deprecated methods as well.

For example, the first 3 steps in the first list above can be done simply by inserting the following lines immediately after the call to warnings.warn in the size method:

        return self._size

    @property
    def _size(self) -> float:

Afterwards, copy and paste the docstring from the size method to _size property.

[mfisher87]

Hah, good point. Didn't even think that far out 🙃 I think this is the only good way forward, any other solution is wildly hacky... :X (anyone want to subclass float so it's callable? 😆)

This means no transition period for users to migrate from .size() to .size. If only we had named the method .get_size(), this would be much more reasonable for our users.

Insert "Why can't you just be normal" meme 😁

This makes the warning message harder to write.

Due to language constraints, you can't transition to .size now, but in earthaccess 1.0, you will be able to. At that same time, .size() will cease to function. We're sowwy 🥺

🤷

I don't think we should encourage users to use a private attribute. And I don't like the idea of a two-step transition. And I don't like that users will just have to deal with the warnings until the next release. There's very little to like about this, but to get to a state that we can like, we have to pay the toll. 😭

[trey-stafford]

Hah, good point. Didn't even think that far out 🙃

💯 . Thanks @chuckwondo 😅 . Agreed with your proposed approach.

[andypbarrett]

I can implement this. Watch for a PR.

[asteiker]

Thanks, all! @mfisher87 We don't have a blocking label but I did add this to our v1.0 milestone. I know that's not perfect since I believe we want this updated before v1.0 but we have other milestone issues that have already been closed out. So I think it's a "good enough" solution w/o proliferating our labels too much.