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

Memo schema tests #253

Merged
merged 10 commits into from
Dec 6, 2021
Merged

Memo schema tests #253

merged 10 commits into from
Dec 6, 2021

Conversation

ggirotto
Copy link
Contributor

@ggirotto ggirotto commented Dec 2, 2021

No description provided.

@ggirotto ggirotto added enhancement New feature or changes to an existing one target: firebase Relates to Firebase and its sub-dependencies labels Dec 2, 2021
@ggirotto ggirotto self-assigned this Dec 2, 2021
@ggirotto ggirotto requested a review from matuella December 2, 2021 16:59
readonly id: string;
readonly question: Record<string, unknown>[];
readonly answer: Record<string, unknown>[];
readonly uniqueId: string;
Copy link
Contributor Author

@ggirotto ggirotto Dec 2, 2021

Choose a reason for hiding this comment

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

Just to point that I personally prefer id over uniqueId as I said when we were developing Flutter client, but we need to create a standard. Or everything becomes id or uniqueId. We already have uniqueId set as the property in the flutter models and collections json's

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, and that's why I've created all of these structures using id, and not uniqueId. What needs to be updated are the collections, and not our current structures. uniqueId made sense where collections were stored locally and shared a unique id amongst all of them, even though they didn't belong to the same collection.

Given that, you can rollback to id no problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But should we update the collections jsons and also Flutter model to use id as well? Otherwise we'll use id in the server and uniqueId in the client. I highlight what I've said back in the client development that id implicitly means that's an unique identifier IMO, that's no reason to emphasis the unique stuff.

PS: We would need this minor refactoring in a separate PR instead mixing with this one stuff

Copy link
Contributor

@matuella matuella left a comment

Choose a reason for hiding this comment

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

Tests looking good!

readonly id: string;
readonly question: Record<string, unknown>[];
readonly answer: Record<string, unknown>[];
readonly uniqueId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, and that's why I've created all of these structures using id, and not uniqueId. What needs to be updated are the collections, and not our current structures. uniqueId made sense where collections were stored locally and shared a unique id amongst all of them, even though they didn't belong to the same collection.

Given that, you can rollback to id no problem.

}

const memoContentValidationSchema = Joi.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const memoContentValidationSchema = Joi.object({
const memoQuillValidationSchema = Joi.object({

Just to enforce where we are taking these from. I think being specific here is beneficial.

Maybe also add a succinct doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to enforce where we are taking these from. I think being specific here is beneficial.

But aren't we going to follow the same format when we develop our own editor in #223? If yes, I don't see why would we enforce quill in the name here, since we plan to remove it in the near future

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to create a custom format, it'll probably use quill deltas anyways, as it's a pretty solid and well-tested approach. Creating a brand new complex-object serialization + data-format is too much work. All available editors, if I'm not mistaken, use quill (i.e zefyr and flutter-quill).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

@ggirotto ggirotto requested a review from matuella December 3, 2021 17:03
@ggirotto ggirotto force-pushed the path-rework-imports branch from 5826536 to 99f15a6 Compare December 3, 2021 17:18
@ggirotto ggirotto force-pushed the memo-schema-tests branch 2 times, most recently from ef3186b to f964d5b Compare December 3, 2021 17:21
Copy link
Contributor

@matuella matuella left a comment

Choose a reason for hiding this comment

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

But should we update the collections jsons and also Flutter model to use id as well? Otherwise we'll use id in the server and uniqueId in the client. I highlight what I've said back in the client development that id implicitly means that's a unique identifier IMO, that's no reason to emphasis the unique stuff.

PS: We would need this minor refactoring in a separate PR instead mixing with this one stuff

I don't quite follow what you meant with "collections jsons", but yes, everything needs to be id since when we opted to update the strategy to use sub-collections of non-conflicting id's of these memos. The client will be eventually updated when the referencing models are mapped from firestore, as it's now mapping from flutter's assets folder + sembast local database.

}

const memoContentValidationSchema = Joi.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to create a custom format, it'll probably use quill deltas anyways, as it's a pretty solid and well-tested approach. Creating a brand new complex-object serialization + data-format is too much work. All available editors, if I'm not mistaken, use quill (i.e zefyr and flutter-quill).

@ggirotto ggirotto force-pushed the path-rework-imports branch from 42bbf70 to e95dbf9 Compare December 3, 2021 19:31
@ggirotto ggirotto changed the base branch from path-rework-imports to sync-script December 3, 2021 19:35
@ggirotto ggirotto requested a review from matuella December 3, 2021 19:37
Copy link
Contributor

@matuella matuella left a comment

Choose a reason for hiding this comment

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

Lgtm

@ggirotto ggirotto merged commit 23782d7 into sync-script Dec 6, 2021
@ggirotto ggirotto deleted the memo-schema-tests branch December 8, 2021 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or changes to an existing one target: firebase Relates to Firebase and its sub-dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants