Added status method for checking status of NASA APIs

[Sherwin-14]

I've added the status function as discussed here. Resolves #161

---

📚 Documentation preview 📚: https://earthaccess--1039.org.readthedocs.build/en/1039/

[github-actions]

👈 Launch a binder notebook on this branch for commit 91e1af9

I will automatically update this comment whenever this PR is modified

👈 Launch a binder notebook on this branch for commit 51e7752

👈 Launch a binder notebook on this branch for commit d79555e

👈 Launch a binder notebook on this branch for commit 6b1032c

👈 Launch a binder notebook on this branch for commit 3b300d0

👈 Launch a binder notebook on this branch for commit f653136

👈 Launch a binder notebook on this branch for commit ed0dc2d

👈 Launch a binder notebook on this branch for commit a77bb77

👈 Launch a binder notebook on this branch for commit f57d653

👈 Launch a binder notebook on this branch for commit ed188a9

👈 Launch a binder notebook on this branch for commit dd6f06c

👈 Launch a binder notebook on this branch for commit 3c51573

👈 Launch a binder notebook on this branch for commit 63d4705

👈 Launch a binder notebook on this branch for commit c28d581

👈 Launch a binder notebook on this branch for commit cd4df7c

👈 Launch a binder notebook on this branch for commit 7490ef9

👈 Launch a binder notebook on this branch for commit 9098457

👈 Launch a binder notebook on this branch for commit a5fc776

👈 Launch a binder notebook on this branch for commit 1c411e3

👈 Launch a binder notebook on this branch for commit 39de816

👈 Launch a binder notebook on this branch for commit 4a256c8

👈 Launch a binder notebook on this branch for commit 7ca5a37

👈 Launch a binder notebook on this branch for commit 32f4ba9

👈 Launch a binder notebook on this branch for commit d7019e1

👈 Launch a binder notebook on this branch for commit 75c8e38

👈 Launch a binder notebook on this branch for commit 77b9e05

👈 Launch a binder notebook on this branch for commit a725e83

👈 Launch a binder notebook on this branch for commit 062c721

👈 Launch a binder notebook on this branch for commit dbd3ac1

👈 Launch a binder notebook on this branch for commit e7d4f4c

👈 Launch a binder notebook on this branch for commit 707c208

👈 Launch a binder notebook on this branch for commit a6c0da0

👈 Launch a binder notebook on this branch for commit 8be2da5

👈 Launch a binder notebook on this branch for commit 2f5abab

👈 Launch a binder notebook on this branch for commit 892e8df

👈 Launch a binder notebook on this branch for commit 8e1c26b

👈 Launch a binder notebook on this branch for commit 7d08cfc

[chuckwondo]

Thanks for picking this up @Sherwin-14. Please let me know if you need any help with making use of the responses library that I mentioned in a comment on your unit test.

[chuckwondo]

Suggested change

	def status(system: System = PROD) -> Dict[str, str] | None:
	def status(system: System = PROD) -> Mapping[str, str]:

[mfisher87]

I hate to go back and forth on this, but I think our return types should be exact -- i.e. this function returns a dict, a more specific case of Mapping.

I like the idea of having looser, more general argument types that describe what we do with the thing passed in, i.e. if we do string lookups on an argument object, we can accept any Mapping object from the user.

But I think it should be the opposite for return types -- tell the user exactly what they're getting from the code they don't control.

[chuckwondo]

I have to disagree with you here. The return type should indicate the "interface" we're returning to the user, thus giving us the freedom to change the implementation without introducing a breaking change.

Of course, in this specific case, the impact is certainly not critical, but generally speaking, this is my preference.

[mfisher87]

My concern is that we have no idea what the user is doing with the returned object. If we type our return loosely, then changing the object we return would be a breaking change even though we don't have to update the annotation. Having a concrete annotation wouldn't change whether the change is breaking or not; it just changes what we communicate to the user, and even helps users recognize and adapt to breaking changes if they use a type checker!

The official Python typing best practices recommend this approach:

https://typing.python.org/en/latest/reference/best_practices.html#arguments-and-return-types

[chuckwondo]

My concern is that we have no idea what the user is doing with the returned object. If we type our return loosely, then changing the object we return would be a breaking change even though we don't have to update the annotation.

Sure, we don't know what the user will do with the returned value, but that's the point of specifying an interface.

By specifying an interface, our "contract" to the user is that we will always return a value that implements the interface. Thus, if the user chooses to rely upon our returning a specific implementation of that interface, they do so at their own risk because the contract makes no promise as to the specific implementation returned.

In other words, by specifying an interface, changing the concrete type we return does not represent a breaking change. The user should not rely on the concrete type, nor should they need to. The interface specifies the behavior users can expect without any need for them to care about how that behavior is implemented.

Only if we were to decide to change which interface we return would we be introducing a breaking change.

Having a concrete annotation wouldn't change whether the change is breaking or not; it just changes what we communicate to the user, and even helps users recognize and adapt to breaking changes if they use a type checker!

Yes, it would. By specifying an "interface," we can change the implementation of that interface without having to communicate anything at all to the user, because we have not broken our contract.

Further, type checking remains completely unaffected because the type signature remains unchanged.

The official Python typing best practices recommend this approach:

https://typing.python.org/en/latest/reference/best_practices.html#arguments-and-return-types

Unfortunately, that "best practice" guide doesn't explain why it recommends the following:

For return values, prefer concrete types (list, dict, etc.) for concrete implementations.

I strongly disagree with that bit, precisely because of what I explained above.

[mfisher87]

I think I get you -- I also read a StackOverflow post where you were having a similar conversation with another commenter. I want to change your mind on this, but I respect that you're an expert, and we can have different opinions informed by our distinct experiences and that's OK. I'm not completely closed to having my mind changed, but I think I understand why this best practice is defined as it is. It's a shame the document doesn't explain itself.

Unfortunately, that "best practice" guide doesn't explain why it recommends the following:

For return values, prefer concrete types (list, dict, etc.) for concrete implementations.

If we don't come to agreement and nobody else weighs in on this, I'd like to resolve this discussion by deferring to the documented best practice, if that's OK with you!

My attempt to change your mind:

semver.org does not provide an official definition of "breaking change" 😭 This feels like another missed opportunity.

The way I define "breaking change" is "doing something that can cause code in the wild to cease functioning". Or "a change that will require any users to make a change in response". I think our definitions may be different. It would be helpful if you could provide your definition!

To me, the purpose of classifying changes as "breaking" or "not breaking" is to notify users when they might need to make those changes in response to software releases. And it gives us a framework to understand how we can aid those users in adapting.

So, for example, if we return an object which is an instance of Foo which conforms to the Mapping<str, str> type, but that object also has some other methods on it like .bar(), users may be using .bar() in their code. If we changed from returning a Foo to returning a dict, users who use .bar() on the object we return will have their code break.

Only by annotating the return with a concrete type can we communicate to the user that we are going to break their code.

Because our code is a black box to many users, I feel the logical conclusion of all of this is that we should tell the user exactly what we need from their inputs (i.e. the loosest possible type as determined by what we're doing inside the black box, where we have control), and tell them exactly what they should expect to get out on the other side of the black box, as what they do with that thing is outside of our control, and changing that thing means they may need to handle it differently.

This means that we would be potentially subject to more frequent changes in what we're advertising the user to expect from our code, but with the benefit that we're giving them much more information about what to expect and more effectively protecting them from frustration and breakage.

To me, the tradeoff is a cost to smaller number of people (maintenance burden of having a definition of "breaking change" that is more strict) with a payoff for a larger number of people (greater knowledge of how our changes will affect their code).

What do you think?

[chuckwondo]

I think we'll just have to agree to disagree here (although to @jhkennedy's point, in Python, this point is indeed somewhat moot given that dict is effectively the only mapping type anybody deals with).

Given that this particular case is very low impact, I'll concede to being outvoted.

If this topic should arise again, we can open a discussion, since this is more about the general topic, not something blocking this particular piece of code.

[mfisher87]

Sounds like a plan. Although I do still want to know if you agree with my definition of "breaking change", and if not, what definition you're using!

[chuckwondo]

I classify a breaking change as a change that breaks (changes) the "contract" defined by our public API. The public API communicates to the user what we expect from the user and what we provide to the user. There are no guarantees to the user for anything outside of the public API.

If we break (change) the contract (e.g., change the type of an input or an output in a non-backwards compatible way), it's a breaking change because it has the potential to break user code that fully adheres to the previous contract.

If the user breaks the contract (e.g., passes the wrong type of value for an input, or treats an output as a type that is not compatible with what's defined in the public API), that's a bug in their code (if it causes their code to break or behave unexpectedly).

Conversely, when a user relies on something that is not part of the public API, they do so at their own risk, meaning that if we make a change to non-public code that causes a break in the user's code that is directly relying on such non-public code, that's the user's problem. Thus, it is not classified as a breaking change.

Not to go back down the rabbit hole, but this is why I'm in favor of return types as interfaces, because the interfaces are part of that public API contract, but the concrete implementations are not (in cases where we expose interfaces as part of the public API, rather than concrete types), meaning that we can change the implementation without breaking the contract (as long as the implementation still implements the interface), and thus not introduce a breaking change.

[mfisher87]

Thanks for providing some more detail about your position. I have lots of thoughts and would like to continue the discussion, but I don't want to dive deeper here.

[itcarroll]

I'd like to request a (new?) page in the docs under "User Guide" for this new top-level function, please.

[itcarroll]

Thank you for the docs page!

[mfisher87]

This is looking great, thanks so much for the docs!

Something I think we should address in the future

Currently the API docs for status end up on the page for "Search and Access".

That's due to this line:

https://github.com/nsidc/earthaccess/blob/b36712bf54d36e5c4e5a71c042a1f465eb3f6b4f/mkdocs.yml#L98-L99

I think we should separate this out, but right now we have one page in the "api" folder, and its name is "api". I feel we should break it up in to categories, e.g. "search", "access", "auth", and "status".

For this PR, I don't think it makes sense to figure this out. It's a pre-existing problem... let's open an issue :)

[chuckwondo]

@Sherwin-14, please merge main into your branch to get your branch up to date.

[chuckwondo]

@Sherwin-14, just a couple more fixes to error messages, but you can skip writing the extra tests we had discussed before.

[mfisher87]

@betolink are the tests failing because a new set of ECS collections have been migrated to the cloud? Do we need to re-generate our collection lists?

[mfisher87]

See https://github.com/nsidc/earthaccess/pull/1061/files#r2257844461 for integration test failure details.

Let's update the integration tests on main and then merge this PR after? or I'm fine with merging it now knowing the reason for the integration tests failure.

@chuckwondo any last minute thoughts?

[chuckwondo]

See https://github.com/nsidc/earthaccess/pull/1061/files#r2257844461 for integration test failure details.

Let's update the integration tests on main and then merge this PR after? or I'm fine with merging it now knowing the reason for the integration tests failure.

@chuckwondo any last minute thoughts?

Either way is fine with me.

[mfisher87]

Alright let's get this PR off the board. Thank you, @Sherwin-14 !!

resolved