-
Notifications
You must be signed in to change notification settings - Fork 650
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
Migrate SharedEntity, LayerNodeStyle and ExternalSource to ActiveRecord #15920
Conversation
9b32222
to
8da8a93
Compare
8da8a93
to
680a34d
Compare
@@ -15,7 +15,7 @@ jobs: | |||
shell: bash | |||
run: | | |||
# This script will exit if any of these files is found in the head branch. | |||
ORM_FILES=$(git grep -l 'class.*Sequel::Model' -- app/models) | |||
ORM_FILES=$(git grep -l 'class.*< Sequel::Model\|class DBService' -- app/models) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prevents some models which include the Sequel::Model
string (but don't inherit from it) to be flagged by the ORM check.
Also, considers DBService
a "Sequel model", as it relies heavily on ::User
9f87031
to
edf9ab9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Migrates several small models from
Sequel
toActiveRecord
.There are several linter warnings about the usage of instance variables within specs, but I'd appreciate if I could leave it like that in order to avoid doing too many spec refactoring.
Acceptance
I've checked in staging: