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

docs: add concepts guide on Datatypes and Datashapes #8557

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

NickCrews
Copy link
Contributor

inspired by #8358

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

Quick pass, but this is really nice @NickCrews -- thanks for putting it together! I've flagged a few typos and some styling nits, nothing major.

I do think that most of the code examples can be made executable (I'm not sure about the one that raises the RelationError...)

docs/concepts/datatypes_datashapes.qmd Outdated Show resolved Hide resolved
docs/concepts/datatypes_datashapes.qmd Outdated Show resolved Hide resolved
docs/concepts/datatypes_datashapes.qmd Outdated Show resolved Hide resolved
docs/concepts/datatypes_datashapes.qmd Outdated Show resolved Hide resolved
docs/concepts/datatypes_datashapes.qmd Outdated Show resolved Hide resolved
docs/concepts/datatypes_datashapes.qmd Outdated Show resolved Hide resolved
@lostmygithubaccount lostmygithubaccount added docs Documentation related issues or PRs docs-preview Add this label to trigger a docs preview labels Mar 5, 2024
@ibis-docs-bot ibis-docs-bot bot removed the docs-preview Add this label to trigger a docs preview label Mar 5, 2024
@ibis-docs-bot
Copy link

ibis-docs-bot bot commented Mar 5, 2024

@lostmygithubaccount
Copy link
Member

I would make the code blocks runnable; use a try/except to catch the error and print it out

@cpcloud
Copy link
Member

cpcloud commented Mar 5, 2024

You can also set error: true for a given cell

NickCrews added a commit to NickCrews/ibis that referenced this pull request Mar 6, 2024
NickCrews added a commit to NickCrews/ibis that referenced this pull request Mar 6, 2024
NickCrews added a commit to NickCrews/ibis that referenced this pull request Mar 6, 2024
@lostmygithubaccount lostmygithubaccount added the docs-preview Add this label to trigger a docs preview label Mar 6, 2024
@ibis-docs-bot ibis-docs-bot bot removed the docs-preview Add this label to trigger a docs preview label Mar 6, 2024
@NickCrews
Copy link
Contributor Author

Thanks for all the thoughts! I agree with all of them.

Two lingering thoughts:

  1. I totally ignored Tables. This could be read that Tables also have a datatype and datashape, which is wrong. But I thought it might just be confusing? if you have a good way of putting that in there, would be great
  2. curious what you think about how I described "flavor". Is that the term we want to use? Are there other aspects of that that are important? I almost left it out, but I think it's not too distracting.

NickCrews added a commit to NickCrews/ibis that referenced this pull request Mar 6, 2024
@ibis-docs-bot
Copy link

ibis-docs-bot bot commented Mar 6, 2024

@lostmygithubaccount lostmygithubaccount added the docs-preview Add this label to trigger a docs preview label Mar 6, 2024
@ibis-docs-bot ibis-docs-bot bot removed the docs-preview Add this label to trigger a docs preview label Mar 6, 2024
@ibis-docs-bot
Copy link

ibis-docs-bot bot commented Mar 6, 2024

@gforsyth
Copy link
Member

gforsyth commented Mar 6, 2024

  1. I totally ignored Tables. This could be read that Tables also have a datatype and datashape, which is wrong. But I thought it might just be confusing? if you have a good way of putting that in there, would be great

I think this is ok to ignore -- if you wanted to be explicit and make sure it's covered, I think I would introduce it in a callout note and say something like (unpolished):

A table is a collection of one or more columns. Because each column can have a different datatype and the number of columns is highly variable, a table itself does not have a datatype or a datashape.  

2. curious what you think about how I described "flavor". Is that the term we want to use? Are there other aspects of that that are important? I almost left it out, but I think it's not too distracting.

I liked it and I think it's a good balance of a word that will both communicate the idea to readers less familiar with datatypes but still correct enough for more familiar readers.

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

I think this can go in, but I'll hold off on merging in case you have any other tweaks you'd like to make @NickCrews . Thanks again for putting this together!

@NickCrews
Copy link
Contributor Author

Thanks for the reviews! This looks good to me, merge it!

@gforsyth gforsyth merged commit cbeb6cf into ibis-project:main Mar 6, 2024
17 checks passed
@cpcloud
Copy link
Member

cpcloud commented Mar 6, 2024

Gah, definitely meant to get in a review of this, but it was minor so I'll just put up a PR to change.

gforsyth pushed a commit that referenced this pull request Mar 6, 2024
Quick follow-up to #8557.

Changes the name of datatypes_datashapes.qmd to just datatypes to
simplify the url.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related issues or PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants