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

Adding beginner friendly precisions #234

Merged
merged 5 commits into from
Jul 6, 2021

Conversation

MilanCorreggio
Copy link
Contributor

No description provided.

@feeblefakie
Copy link
Contributor

@MilanCorreggio Thank you for the PR. For Scalar DB PRs, please add @brfrn169 and me and we will add other reviewers additionally as needed.

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

Thank you for raising this PR! Left some comments. Please check them!

docs/getting-started-with-scalardb.md Outdated Show resolved Hide resolved
docs/getting-started-with-scalardb.md Outdated Show resolved Hide resolved
Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

Thank you very much for pointing them out and suggestions.
I made additional suggestions.

docs/getting-started-with-scalardb.md Outdated Show resolved Hide resolved
@@ -147,6 +149,7 @@ $ ../../gradlew run --args="-mode storage -action pay -amount 100 -to merchant1
## Set up database schema with transaction

To apply transaction, we can just add a key `transaction` and value as `true` in Scalar DB scheme.
You can modify the json file `emoney-storage.json` by the json object bellow.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is emoney-transaction.json so we don't need this line.
Instead, I think we should say so.

Suggested change
You can modify the json file `emoney-storage.json` by the json object bellow.

Copy link
Contributor

Choose a reason for hiding this comment

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

@feeblefakie san,
Sorry to disturb you.
Can we modify it like You can create a JSON file emoney-transaction.json by the JSON object below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this line we already created the JSON file. We are doing the second test using transacion as true, so i suppose that modify is ok here.

docs/getting-started-with-scalardb.md Outdated Show resolved Hide resolved
docs/getting-started-with-scalardb.md Outdated Show resolved Hide resolved
Copy link
Contributor

@scalar-boney scalar-boney left a comment

Choose a reason for hiding this comment

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

The document is tested with Cosmos DB, It is working fine.

@scalar-boney scalar-boney self-requested a review July 5, 2021 14:52
scalar-boney
scalar-boney previously approved these changes Jul 5, 2021
Copy link
Contributor

@scalar-boney scalar-boney left a comment

Choose a reason for hiding this comment

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

LGTM except for the above modifications.

brfrn169
brfrn169 previously approved these changes Jul 6, 2021
Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

Sorry for the confusion! I thought there are files named emoney-storage.json and emoney-transaction.json.
I added more comments.
Mostly for English correction and clarification.

@@ -30,6 +30,7 @@ $ cd docs/getting-started

First of all, you need to define how the data will be organized (a.k.a database schema) in the application with Scalar DB database schema.
Here is a database schema for the sample application. For the supported data types, please see [this doc](schema.md) for more details.
You can create a JSON file `emoney-transaction.json` by the JSON object below.
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
You can create a JSON file `emoney-transaction.json` by the JSON object below.
You can create a JSON file `emoney-storage.json` with the JSON below.

Since it's for storage.

@@ -144,9 +145,10 @@ $ ../../gradlew run --args="-mode storage -action charge -amount 0 -to merchant1
$ ../../gradlew run --args="-mode storage -action pay -amount 100 -to merchant1 -from user1"
```

## Set up database schema with transaction
## Set up database schema for transaction

To apply transaction, we can just add a key `transaction` and value as `true` in Scalar DB scheme.
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
To apply transaction, we can just add a key `transaction` and value as `true` in Scalar DB scheme.
To use transaction, we can just add a key `transaction` and value as `true` in the Scalar DB scheme we used.


To apply transaction, we can just add a key `transaction` and value as `true` in Scalar DB scheme.
You can modify the JSON file `emoney-storage.json` by the json object bellow.
Copy link
Contributor

@feeblefakie feeblefakie Jul 6, 2021

Choose a reason for hiding this comment

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

Suggested change
You can modify the JSON file `emoney-storage.json` by the json object bellow.
You can create the JSON a `emoney-transaction.json` with the JSON below.

We are using emoney-transaction.json below so I think we should mention the name.

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@feeblefakie feeblefakie merged commit 4dee38b into master Jul 6, 2021
@feeblefakie feeblefakie deleted the Update_getting-started_documentation branch July 6, 2021 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants