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

Validate documentation of usage ActiveRecord #473

Merged
merged 4 commits into from
Feb 14, 2023
Merged

Conversation

miry
Copy link
Contributor

@miry miry commented Feb 8, 2023

Update README about a new adapter.

@miry miry mentioned this pull request Feb 8, 2023
4 tasks
@miry miry self-assigned this Feb 8, 2023
@miry miry added the Semian label Feb 8, 2023
@miry miry marked this pull request as ready for review February 13, 2023 15:37
@miry miry requested review from a team, bradfeehan, keldin-coding and ShopifyDotIo and removed request for a team February 13, 2023 15:38
@miry miry changed the title Validate documentation of usage AR Trilogy Validate documentation of usage ActiveRecord Feb 13, 2023
Copy link

@AbdulRahmanAlHamali AbdulRahmanAlHamali left a comment

Choose a reason for hiding this comment

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

small comment but otherwise LGTM

README.md Outdated
@@ -299,6 +300,32 @@ SEMIAN_PARAMETERS = { tickets: 1,
open_circuit_server_errors: true }
```

#### ActiveRecord

Semian supports two Activeredord adapters: `mysql2` and `trilogy`.

Choose a reason for hiding this comment

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

Suggested change
Semian supports two Activeredord adapters: `mysql2` and `trilogy`.
Semian supports two ActiveRecord adapters: `mysql2` and `trilogy`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, mysql2 is not actually supported as an Active Record adapter right now -- mysql2 has a Semian adapter, but we're targeting the client library directly (nothing to do with Active Record). We should probably just mention trilogy for now, because a non-Rails app could use the mysql2 adapter without having Active Record (as is the case with SFR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adrianna-chang-shopify The mysql2 semian still could be configured via database.yml.
I probably used a wrong expression.

Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

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

Some small comments.

Does it also make sense to reference the example cases in this part of the README?
e.g.

Example cases for `activerecord-trilogy-adapter` can be run using
`BUNDLE_GEMFILE=gemfiles/activerecord_trilogy_adapter.gemfile bundle exec rake examples:activerecord_trilogy_adapter`

README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -299,6 +300,32 @@ SEMIAN_PARAMETERS = { tickets: 1,
open_circuit_server_errors: true }
```

#### ActiveRecord

Semian supports two Activeredord adapters: `mysql2` and `trilogy`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, mysql2 is not actually supported as an Active Record adapter right now -- mysql2 has a Semian adapter, but we're targeting the client library directly (nothing to do with Active Record). We should probably just mention trilogy for now, because a non-Rails app could use the mysql2 adapter without having Active Record (as is the case with SFR).

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@miry miry merged commit d489262 into master Feb 14, 2023
@miry miry deleted the 473-ar-trilogy-adapter-doc branch February 14, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants