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

391 document indexing and table store #397

Merged
merged 5 commits into from
Jun 5, 2024

Conversation

pdelboca
Copy link
Member

@pdelboca pdelboca commented May 23, 2024

Fixes #391

This PR is part of understanding and documenting the indexing flow of tabular data.

@roll and @guergana I'm proposing this idea to start documenting what are the properties and when are they used. I think will improve a lot the readability of the codebase.

I couldn't find any reference nor use of the error?: types.IError property. @roll correct me if I'm wrong.

Copy link

cloudflare-pages bot commented May 23, 2024

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 47f3653
Status: ✅  Deploy successful!
Preview URL: https://7a2abb1e.opendataeditor.pages.dev
Branch Preview URL: https://391-document-indexing-and-ta.opendataeditor.pages.dev

View logs

@pdelboca pdelboca requested review from roll and guergana May 23, 2024 07:47
@pdelboca pdelboca mentioned this pull request May 23, 2024
4 tasks
@guergana
Copy link
Collaborator

guergana commented May 24, 2024

Hello, @pdelboca I think it's a really good idea to document everything but I would suggest we write the comments as JSDoc annotations (Typescript seems to come with JSDoc support): https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html or use another library for code annotations in Typescript.

The advantages of commenting in a standarized way is that it will be easier for developers familiar with JSDoc, there are clear docs of what JSDoc annotations mean and the annotations could be later included in a script to automatically generate docs. The con is that we need to learn the JSDoc syntax if we wish to go beyond writing basic comments. But I think the advantages outweigh the disadvantages.

I mention JSDoc because it is the one I am most familiar with, but there seem to be others like Typedoc, and TSDoc and probably others. In case we decide to take this route we need to make an investigation and decide which library would better suit our needs for the project. :)

If you think it's too much I am also happy to go with the simple comments, but we would still benefit from writing the comments in JSDoc format in case we change our minds in the future (meaning we can start off with JSDoc comments in the simplest format and then expand to add more complex JSDoc annotations)

/* This is a normal comment */
/** This is a JSDoc comment */

You can see some simple examples here

Copy link
Collaborator

@guergana guergana left a comment

Choose a reason for hiding this comment

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

I've added my review here: #397 (comment)

@pdelboca
Copy link
Member Author

@guergana I agree with a better syntax, I migrated to the simple comment structure /** comment **/.

The idea is to have a way to start adding comments as we understand the code base. We will have a proper time to document at the end of the project so let's start with the simplest way possible :)

Copy link
Collaborator

@guergana guergana left a comment

Choose a reason for hiding this comment

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

I was researching a bit more and this indeed is the correct way of adding jsDocs comments to TS interfaces but it might be too complex in the end to use JSDocs. It doesn't do any damage though to have the comments in JSDocs format in any case 😅

@roll
Copy link
Collaborator

roll commented May 27, 2024

@pdelboca
If TypeScript lets you something to remove -- than it's not used =)

@pdelboca pdelboca merged commit 9f7e94f into main Jun 5, 2024
8 checks passed
@pdelboca pdelboca deleted the 391-document-indexing-and-table-store branch June 5, 2024 11:56
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.

Check in the INDEX function - Research
3 participants