Amend Documents #10

mtshr · 2024-04-10T09:27:56Z

The pull request fixes some typos.

kenkoooo

Thank you for your Pull Request! It looks good to me.

During the code review, I noticed something we can fix, so let me leave some comments.

kenkoooo · 2024-04-10T15:01:52Z

README.md

@@ -11,16 +11,16 @@

 cder (_see-der_) is a database seeding tool to help you import fixure data in your local environment.

-Generating seeds programmatically is a easy task, but maintaining them is not.
+Generating seeds programmatically is an easy task, but maintaining them is not.
 Everytime when your schema is changed, your seed can be broken.


Suggested change

Everytime when your schema is changed, your seed can be broken.

Every time when your schema is changed, your seed can be broken.

kenkoooo · 2024-04-10T15:02:23Z

README.md

@@ -11,16 +11,16 @@

 cder (_see-der_) is a database seeding tool to help you import fixure data in your local environment.

-Generating seeds programmatically is a easy task, but maintaining them is not.
+Generating seeds programmatically is an easy task, but maintaining them is not.
 Everytime when your schema is changed, your seed can be broken.
 It costs your team extra efforts to keep them updated.


Suggested change

It costs your team extra efforts to keep them updated.

It costs your team extra effort to keep them updated.

I fixed as suggested, but may I ask for an explanation in detail for future reference, if you would not mind?
For clarity, just let me tell what I thought: efforts here referred to humans' 'actions' required to update seeds, that is, rewriting or adding seeds data and keeping them new, and therefore [countable] an attempt to do something especially when it is difficult to do [1] sounded suited to me.
[1] https://www.oxfordlearnersdictionaries.com/definition/english/effort

That makes sense to me (actually my initial intention).
Having said that, it's not a critical point in terms of usage of this crate, so I'd accept either way (please correct if you will!) Thanks again for the detailed review 👍

kenkoooo · 2024-04-10T15:02:46Z

README.md

 Everytime when your schema is changed, your seed can be broken.
 It costs your team extra efforts to keep them updated.

 #### with cder you can:
 - maintain your data in a readable format, separated from the seeding program
 - handle reference integrities on-the-fly, using **embedded tags**
- reuse existing struct and insert function, with only a little glue code is needed
+- reuse existing structs and insert function, with only a little glue code is needed


Suggested change

- reuse existing structs and insert function, with only a little glue code is needed

- reuse existing structs and insert function,s with only a little glue code is needed

@kenkoooo perhaps you meant s comes into before comma, right?

Suggested change

- reuse existing structs and insert function, with only a little glue code is needed

- reuse existing structs and insert functions, with only a little glue code is needed

Thanks! Good catch!

kenkoooo · 2024-04-10T15:05:14Z

README.md

@@ -250,12 +250,12 @@ Dev:
  email: ${{ ENV(DEVELOPER_EMAIL:-"[email protected]") }}
 ```

-Without specifying the defalut value, all the tags that points to undefined environment vars are simply replaced by empty string "".
+Without specifying the defalut value, all the tags that point to undefined environment vars are simply replaced by empty string "".


Suggested change

Without specifying the defalut value, all the tags that point to undefined environment vars are simply replaced by empty string "".

Without specifying the default value, all the tags that point to undefined environment vars are simply replaced by empty string "".

kenkoooo · 2024-04-10T15:06:25Z

README.md


 ### Data representation
 cder deserializes yaml data based on [serde-yaml](https://github.com/dtolnay/serde-yaml), that supports powerful [serde serialization framework](https://serde.rs/). With serde, you can deserialize pretty much any struct. You can see a few [sample structs](tests/test_utils/types.rs) with various types of attributes and [the yaml files](tests/fixtures) that can be used as their seeds.

-Below shows a few basics of required YAML format.
+Shown below are a few basics of required YAML format.


Suggested change

Shown below are a few basics of required YAML format.

Below are a few basics of required YAML format.

Okay, let us keep things simple here.

fursich · 2024-04-10T15:41:48Z

@mtshr Thank you for the thorough check!
Would appreciate if you add a few more corrections that @kenkoooo suggested - all looks fine to me :)

fursich · 2024-04-10T15:44:01Z

README.md

 Everytime when your schema is changed, your seed can be broken.
 It costs your team extra efforts to keep them updated.

 #### with cder you can:
 - maintain your data in a readable format, separated from the seeding program
 - handle reference integrities on-the-fly, using **embedded tags**
- reuse existing struct and insert function, with only a little glue code is needed
+- reuse existing structs and insert function, with only a little glue code is needed


@kenkoooo perhaps you meant s comes into before comma, right?

Suggested change

- reuse existing structs and insert function, with only a little glue code is needed

- reuse existing structs and insert functions, with only a little glue code is needed

mtshr · 2024-04-10T22:19:34Z

Thank you two for taking a look and @kenkoooo -san for pointing out what I overlooked. I have fixed those as suggested.

While I was reworking I just found a few more I regrettably overlooked to fix. May I ask you to review these too?

fixure to fixture

cder/README.md

Line 12 in fe54c8f

cder (_see-der_) is a database seeding tool to help you import fixture data in your local environment.

cder/README.md

Line 121 in fe54c8f

.populate("fixtures/users.yml", |input| {

cder/src/database_seeder.rs

Line 109 in fe54c8f

/// .populate("fixtures/users.yml", |input| {
seed to seeds

cder/README.md

Line 15 in fe54c8f

Every time when your schema is changed, your seeds can be broken.

mtshr · 2024-04-11T00:37:55Z

@kenkoooo @fursich
It seems that tests/database_seeder_async.rs and tests/database_seeder.rs depend on tests/test_utils/mod.rs's

pub use mock_database::{sort_records_by_ids, MockTable};

but tests/struct_loader.rs does not, and therefore clippy failed.

There may be some ways to resolve the issue by like allowing unused imports or removing the above line and specifying only used identities in tests/*.rs, but what would be desirable and preferable for the crate? It would be helpful to hear your ideas and policy, or any better way I could not hit upon.

fursich · 2024-04-11T04:37:33Z

@mtshr

Thanks for the analysis on CI failure - yes, apparently it's nothing to do with this change (sorry for that..!)

My view on this:

In principal I'd like to respect the default clippy rules. (avoid allow wherever possible)
I originally imported everything with * (which is pretty convenient for tests), but it may not be a good practice.

So my suggestion is to specify all the required imports instead of * in all files under tests/ folder (preferably in a separate PR).
I'll be submitting a PR to fix it, so that you can rebase upon this. Or if you would be willing to create a separate PR, of course I'm happy to accept - let me know!

mtshr · 2024-04-11T05:02:50Z

@fursich

Thank you for the reply. In order to avoid my misinterpretation, may I request you to make a pull request to resolve the problem? I am grateful for your help.

fursich · 2024-04-11T06:13:02Z

@mtshr

Looks like it's a false positive (possibly a bug with latest rustc ??) - and I ended up add a workaround.
Anyway the fix is merged, so would you please rebase your PR? Thank you!

mtshr · 2024-04-11T08:00:48Z

@fursich
Thank you for taking your time to further look into the problem.
I rebased the branch and squashed the commits.

fursich

@mtshr All looks great! thanks a lot for contributing to cder 🎉

mtshr · 2024-04-12T03:56:10Z

Thank you for kind supports!

kenkoooo approved these changes Apr 10, 2024

View reviewed changes

fursich approved these changes Apr 10, 2024

View reviewed changes

mtshr changed the title ~~Amend README~~ Amend Documents Apr 10, 2024

mtshr requested review from kenkoooo and fursich April 10, 2024 22:20

kenkoooo approved these changes Apr 11, 2024

View reviewed changes

fursich mentioned this pull request Apr 11, 2024

workaround: temporarily allow unused_imports warnings with tests #11

Merged

Amend documents

1f06115

fursich approved these changes Apr 12, 2024

View reviewed changes

fursich merged commit 51c2063 into estie-inc:main Apr 12, 2024
6 checks passed

mtshr deleted the fix/amend-readme branch April 12, 2024 04:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amend Documents #10

Amend Documents #10

mtshr commented Apr 10, 2024

kenkoooo left a comment

kenkoooo Apr 10, 2024

kenkoooo Apr 10, 2024

mtshr Apr 10, 2024

fursich Apr 11, 2024

kenkoooo Apr 10, 2024

fursich Apr 10, 2024

kenkoooo Apr 10, 2024

kenkoooo Apr 10, 2024

kenkoooo Apr 10, 2024

mtshr Apr 10, 2024

fursich commented Apr 10, 2024

fursich Apr 10, 2024

mtshr commented Apr 10, 2024

mtshr commented Apr 11, 2024

fursich commented Apr 11, 2024 •

edited

Loading

mtshr commented Apr 11, 2024

fursich commented Apr 11, 2024

mtshr commented Apr 11, 2024

fursich left a comment •

edited

Loading

mtshr commented Apr 12, 2024

	Everytime when your schema is changed, your seed can be broken.
	Every time when your schema is changed, your seed can be broken.

	It costs your team extra efforts to keep them updated.
	It costs your team extra effort to keep them updated.

	- reuse existing structs and insert function, with only a little glue code is needed
	- reuse existing structs and insert function,s with only a little glue code is needed

	Without specifying the defalut value, all the tags that point to undefined environment vars are simply replaced by empty string "".
	Without specifying the default value, all the tags that point to undefined environment vars are simply replaced by empty string "".

	Shown below are a few basics of required YAML format.
	Below are a few basics of required YAML format.

Amend Documents #10

Amend Documents #10

Conversation

mtshr commented Apr 10, 2024

kenkoooo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fursich commented Apr 10, 2024

Choose a reason for hiding this comment

mtshr commented Apr 10, 2024

mtshr commented Apr 11, 2024

fursich commented Apr 11, 2024 • edited Loading

mtshr commented Apr 11, 2024

fursich commented Apr 11, 2024

mtshr commented Apr 11, 2024

fursich left a comment • edited Loading

Choose a reason for hiding this comment

mtshr commented Apr 12, 2024

fursich commented Apr 11, 2024 •

edited

Loading

fursich left a comment •

edited

Loading