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

Stac #34

Merged
merged 11 commits into from
Jan 30, 2024
Merged

Stac #34

merged 11 commits into from
Jan 30, 2024

Conversation

dchandan
Copy link
Collaborator

STAC tutorials.

"metadata": {},
"source": [
"The library that we will use to programmatically interact and query the Marble STAC catalog is called [`pystac-client`](https://pystac-client.readthedocs.io/en/stable/). This is built on the robust [`pystac`](https://pystac.readthedocs.io/en/stable/) library for creating and reading STAC Catalogs, \n",
"and extends that by allowing the ability to work with a STAC API. We highly encourage you to look at the tutorials on the `pystac-client` \n",
Copy link
Collaborator

@alexandercyu alexandercyu Jan 25, 2024

Choose a reason for hiding this comment

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

Take the backticks out of the link

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I follow. Could you explain a bit more?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe @alexandercyu just means that [`pystac-client`] should be [pystac-client]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have been using the convention of code syntax for library names. Unless there is objection to doing that site-wide then I think we can leave this as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a note for the future...

We should make a style guide for this sort of thing so that we can make sure we're consistent across all tutorials

Copy link
Collaborator

Choose a reason for hiding this comment

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

The pystac-client link format has backticks in it and it renders like the other backtick text. It's inconsistent formatting and it doesn't immediately show that it's a link

Copy link
Collaborator

@mishaschwartz mishaschwartz Jan 29, 2024

Choose a reason for hiding this comment

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

To me this paragraph renders like:

image

So we get the backtick version with the link (blue text) vs. the backtick version without the link (pink-ish text).

@alexandercyu are you saying that we should remove the link or the backticks?
And do you mean it is inconsistent with other links or other backticked text?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm saying just remove the backticks. It's as you pointed out: [`pystac-client`] should be [pystac-client]

Copy link
Collaborator

Choose a reason for hiding this comment

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

To me this paragraph renders like:

image

So we get the backtick version with the link (blue text) vs. the backtick version without the link (pink-ish text).

@alexandercyu are you saying that we should remove the link or the backticks? And do you mean it is inconsistent with other links or other backticked text?

I don't know what happened the first time I rendered, but it is now rendering for me like you have shown. I recall seeing the backtick link and the non-backtick link rendering the same. But now that this is resolved I think it's good to go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the clarification @alexandercyu. I was wondering if there was a compilation issue you were facing...

Copy link
Collaborator

@alexandercyu alexandercyu left a comment

Choose a reason for hiding this comment

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

Other than some formatting changes, I think this described the STAC browser very well.

Copy link
Collaborator

@mishaschwartz mishaschwartz left a comment

Choose a reason for hiding this comment

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

Looks great! Here's a first pass review that's mostly just proofreading stuff.

tutorials/users/catalog/catalog.md Outdated Show resolved Hide resolved
tutorials/users/catalog/catalog.md Outdated Show resolved Hide resolved
tutorials/users/catalog/gui-search.md Outdated Show resolved Hide resolved
tutorials/users/catalog/pystac-client.ipynb Outdated Show resolved Hide resolved
tutorials/users/catalog/gui-search.md Outdated Show resolved Hide resolved
tutorials/users/catalog/pystac-client.ipynb Outdated Show resolved Hide resolved
tutorials/users/catalog/pystac-client.ipynb Outdated Show resolved Hide resolved
tutorials/users/catalog/pystac-client.ipynb Show resolved Hide resolved
tutorials/users/catalog/pystac-client.ipynb Show resolved Hide resolved
tutorials/users/catalog/pystac-client.ipynb Outdated Show resolved Hide resolved
@dchandan
Copy link
Collaborator Author

Thanks @alexandercyu and @mishaschwartz. I think I've addressed all the requests. Please take a look.

Copy link
Collaborator

@mishaschwartz mishaschwartz left a comment

Choose a reason for hiding this comment

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

Looks good. Just need to figure out/resolve:

@dchandan
Copy link
Collaborator Author

I think all issues are addressed now.

@dchandan
Copy link
Collaborator Author

Is this ready to merge?

@alexandercyu
Copy link
Collaborator

Is this ready to merge?

I replied to your question about the backticks in the link. After that it will be ready for me.

Copy link
Collaborator

@mishaschwartz mishaschwartz left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@dchandan dchandan merged commit 78cc10a into main Jan 30, 2024
@dchandan dchandan deleted the stac branch January 30, 2024 16:28
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

Successfully merging this pull request may close these issues.

None yet

3 participants