chronomodel icon indicating copy to clipboard operation
chronomodel copied to clipboard

Better check for whether or not a table is_chrono?

Open pnomolos opened this issue 4 years ago • 10 comments

The previous check didn't work properly because data_source_sql (which data_source_exists? uses internally) only qualifies the table_name to the current schema if a fully qualified name isn't passed.

This could be made even better if the schema was stripped off table before it's passed in, as right now this won't work if you pass a fully qualified name in to change_table and also pass temporal: true

Ref: https://apidock.com/rails/v5.1.7/ActiveRecord/ConnectionAdapters/PostgreSQL/SchemaStatements/data_source_sql and https://apidock.com/rails/v5.1.7/ActiveRecord/ConnectionAdapters/PostgreSQL/SchemaStatements/quoted_scope

pnomolos avatar Mar 04 '20 23:03 pnomolos

Looks like this project has begun to be maintained again, so I've merged upstream master. If this PR is still useful would it be possible to get it in the next release?

pnomolos avatar Nov 25 '21 06:11 pnomolos

Hi, any chance to rebase instead of merge and provide a failing test case?

tagliala avatar Nov 25 '21 07:11 tagliala

@tagliala Yes, I should be able to do that. Most likely won't be until next week, though.

pnomolos avatar Nov 25 '21 18:11 pnomolos

@tagliala This is ready for review :)

pnomolos avatar Sep 12 '22 22:09 pnomolos

Hi, I can suggest to run GitHub actions on your fork to fix specs on older ruby and rails versions

NoMethodError:
       undefined method `uuid' for Random:Class

tagliala avatar Sep 16 '22 14:09 tagliala

@tagliala I added a spec that did a great job of highlighting where things currently aren't working (having a "." within the table name) and fixed all the issues I could find around it. I tested locally against AR 5/5.2/6/7 and everything appears to be working fine on those AR versions. I'll push a fix for Random.uuid not being available in older versions of Ruby.

pnomolos avatar Sep 16 '22 19:09 pnomolos

Hi @tagliala following up about this to see where it's at. Thank you :pray: !

pnomolos avatar Oct 13 '22 05:10 pnomolos

Hi @tagliala Following up about this (and #97), thank you!

pnomolos avatar Mar 17 '23 22:03 pnomolos

Any plans on this and #97 get merged? Thank you!

pavlitsky avatar Mar 30 '23 20:03 pavlitsky

Hi, thanks for your interest in ChronoModel

I can't guarantee that this will be merged. We don't want to introduce new issues in this component and all we are fine with this limitation. Also, I'm concerned about new issues that this can introduce by allowing a different schema path

As per the PR itself, I've left a comment. I'm also interested in a better management of quoted chars in the table name, I would like to understand if it is possible to avoid checks like table_name[0] != '"' and

        def quote_identifier_name(prefix: "", table: "", suffix: "")
          if table[0] == '"' && table[-1] == '"'
            "\"#{prefix}#{table[1..-2]}#{suffix}\""
          else
            "#{prefix}#{table}#{suffix}"
          end

tagliala avatar Apr 11 '23 10:04 tagliala