Skip to content

Conversation

@maryamsulemani97
Copy link
Contributor

@maryamsulemani97 maryamsulemani97 commented Mar 23, 2022

This PR moves all the information on the primary key to one dedicated page.

  1. I'm not sure if "Core concepts" is the right place for this guide. Any suggestions are welcome
  2. Did I miss any other errors? I only mentioned the ones in Errors of primary key #233

Copy link
Contributor

@dichotommy dichotommy 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 is a good start. Bringing this information onto its own page is a huge improvement over what we have currently. The next step is to improve the explanation of the primary key to make it less technical. Take the beginning for example:

The primary key is a mandatory attribute linked to a unique value, the document id. It is part of the primary field.

Each index recognizes only one primary key attribute. Once a primary key has been set for an index, it cannot be changed anymore. If no primary key is found in one document, none of the documents will be stored. The primary key ensures that there are no identical documents in the same index.

The language here is overly technical and dense with a lot of terminology right off the bat, and the second paragraph is basically a list of unrelated facts about primary keys. This doesn't really align with the idea of "core concepts". The explanation should start conceptual before getting more specific and technical.

Also, there's still a decent amount of content in the "primary field" section of the documents guide. Once we're sure that this information has been thoroughly copied over to the primary key guide, I would like to delete the content of this section entirely and basically replace it with a single sentence or two linking to the primary key guide.

Also—mentioning this here because it appears multiple times in both guides—it's a bit confusing to say:

"The primary key can only be of type integer or string, composed of alphanumeric characters a-z A-Z 0-9, hyphens -, and underscores _."

This is true of the value of the primary key (what we currently call the document ID). These rules are common for unique IDs. However the primary key itself—what we could call the attribute of the field—can be any valid string (AFAIK). Example:

"Th!s i$ a \"valid\" primary key.": "this-is-a-valid-document-id"
"???": 123456
"'These are also valid primary keys ^'": "number-42_69_420"

Maybe I'm being overly precise here, but to me it's an important distinction 😅

Copy link
Member

@guimachiavelli guimachiavelli 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 is getting closer, but requires more polishing—mostly because the content of the original guides was in dire need of a review.

maryamsulemani97 and others added 2 commits April 6, 2022 20:27
Co-authored-by: gui machiavelli <hey@guimachiavelli.com>
guimachiavelli
guimachiavelli previously approved these changes Apr 7, 2022
Copy link
Member

@guimachiavelli guimachiavelli left a comment

Choose a reason for hiding this comment

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

Two minor text edits, but LGTM 🧚‍♂️

EDIT: Oh, there's also a broken link somewhere, according to our build checker.

Co-authored-by: gui machiavelli <hey@guimachiavelli.com>
@maryamsulemani97
Copy link
Contributor Author

maryamsulemani97 commented Apr 7, 2022

there's also a broken link somewhere, according to our build checker

It's because I have two headings called “Primary key”, the page itself, and a subheading on that page.
I used #primary-key and #primary-key-2 to differentiate between the two, and it works. But the checks fail for #primary-key-2
Ik Tommy used this format in the keys API reference, reference/api/keys.md lines 95 and 200
Do I disable the checks for this PR? Or is there some other solution?

Or I could call the page "The primary key"

@guimachiavelli
Copy link
Member

I'm not sure I like "The primary key" as the page title, but if we can't think of anything else…I guess it's ok? 🤷

If we do go with that, let's only change the page title, but keep the file name without the article.

Copy link
Contributor

@dichotommy dichotommy left a comment

Choose a reason for hiding this comment

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

Great work 👏🏻 🥳 Once these comments have been addressed and merge conflicts have been fixed, I think this is ready to merge 🚀

maryamsulemani97 and others added 2 commits April 13, 2022 20:10
Co-authored-by: Tommy <68053732+dichotommy@users.noreply.github.com>
@maryamsulemani97 maryamsulemani97 linked an issue Apr 13, 2022 that may be closed by this pull request
Copy link
Contributor

@dichotommy dichotommy left a comment

Choose a reason for hiding this comment

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

Requesting some small final changes 🏄🏻

maryamsulemani97 and others added 2 commits April 20, 2022 20:09
Co-authored-by: Tommy <68053732+dichotommy@users.noreply.github.com>
Copy link
Contributor

@dichotommy dichotommy left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome work on this 👏🏻 🧱

@maryamsulemani97
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Apr 26, 2022

Build succeeded:

@bors bors bot merged commit 9f65084 into master Apr 26, 2022
@bors bors bot deleted the new-primary-field-guide branch April 26, 2022 16:29
@maryamsulemani97 maryamsulemani97 mentioned this pull request May 17, 2022
2 tasks
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.

primary key can be changed if index is empty

4 participants