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

Print warning when unknown options are passed to Trilogy#initialize #155

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bensheldon
Copy link
Contributor

Alternative to #151.

I'm not confident I have caught all of the possible configuration options.

end
$stderr.puts "WARNING: Unknown Trilogy options: #{original_options.keys.join(", ")}" unless original_options.empty?
invalid_keys = options.keys - VALID_OPTIONS
$stderr.puts "WARNING: Unknown Trilogy options: #{invalid_keys.keys.join(", ")}" unless original_options.empty?
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 Rails might pass through the whole db config, which includes keys that wouldn't be valid here. We should check that before merging this.

@@ -7,7 +7,18 @@
require "trilogy/encoding"

class Trilogy
VALID_OPTIONS = %i[
host port path database username password encoding
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the distinction between socket and path options?

I see this:

path: DEFAULT_SOCK, # for trilogy
socket: DEFAULT_SOCK, # for mysql2

..but also this:

Copy link
Contributor

@composerinteralia composerinteralia Jan 30, 2024

Choose a reason for hiding this comment

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

I don't think path is a valid option for either trilogy or mysql2. Maybe it used to be called path?

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.

2 participants