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

Add MySql support #9

Merged
merged 28 commits into from
Nov 2, 2023
Merged

Add MySql support #9

merged 28 commits into from
Nov 2, 2023

Conversation

adlamas
Copy link
Contributor

@adlamas adlamas commented Oct 4, 2023

  • Update adapter helper to support MySQL json types
  • Update generator spec to permit using MySql
  • Update migration spec
  • Update CI to check MySql's support

@adlamas adlamas changed the title Feature/add mysql support Add MySql support Oct 30, 2023
@adlamas adlamas marked this pull request as ready for review October 30, 2023 14:37
Copy link
Collaborator

@guillermoap guillermoap left a comment

Choose a reason for hiding this comment

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

Requesting changes mainly because of the missing spec. Looking great 💪🏻

spec/generators/active_outbox_generator_spec.rb Outdated Show resolved Hide resolved
@@ -3,15 +3,25 @@
module ActiveOutbox
module AdapterHelper
def self.uuid_type
postgres? ? 'uuid' : 'string'
return 'uuid' if postgres?
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use case? I feel it's cleaner that returns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the way it would be with case

    def self.uuid_type
      case adapter
      when 'postgres'
        'uuid'
      when 'mysql2'
        'string'
      else
        'string'
      end
    end

    def self.json_type
      case adapter
      when 'postgres'
        'jsonb'
      when 'mysql2'
        'json'
      else
        'string'
      end
    end

    def self.adapter
      ActiveRecord::Base.connection.adapter_name.downcase
    end
  end

Personally, I prefer it with if statements, I don't see it more clear

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@guillermoap
Copy link
Collaborator

Also remember to bump version, and the run a bundle install

@adlamas adlamas closed this Nov 1, 2023
@adlamas adlamas reopened this Nov 1, 2023
@adlamas adlamas changed the base branch from main to refactor-generators November 2, 2023 19:00
@adlamas adlamas changed the base branch from refactor-generators to main November 2, 2023 19:02
@adlamas adlamas closed this Nov 2, 2023
@adlamas adlamas reopened this Nov 2, 2023
@guillermoap guillermoap merged commit 4de84df into main Nov 2, 2023
4 checks passed
@guillermoap guillermoap deleted the feature/add-mysql-support branch November 2, 2023 19:20
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.

None yet

2 participants