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

Use class inst. var. instead of class var. #641

Merged
merged 1 commit into from Nov 8, 2015
Merged

Conversation

jaredbeck
Copy link
Member

I use the term "class instance variable" and "class variable"
even though PaperTrail is a module. This nomenclature is
conventional in ruby.

module A; @@x = :x; puts defined?(@@x); end
#=> "class variable"

Actually, because PaperTrail is a module, the use of a class
variable here is not as problematic as in an actual class, so
this change is solely to conform the the Ruby Style Guide.

Avoid the usage of class (@@) variables due to their "nasty"
behavior in inheritance.
https://github.com/bbatsov/ruby-style-guide#no-class-vars

I use the term "class instance variable" and "class variable"
even though `PaperTrail` is a module. This nomenclature is
conventional in ruby.

> module A; @@x = :x; puts defined?(@@x); end
> #=> "class variable"

Actually, because `PaperTrail` is a module, the use of a class
variable here is not as problematic as in an actual class, so
this change is solely to conform the the Ruby Style Guide.

> Avoid the usage of class (@@) variables due to their "nasty"
> behavior in inheritance.
> https://github.com/bbatsov/ruby-style-guide#no-class-vars

As stated above, because `PaperTrail` is a module, there is no
concern over inheritance, and this change is solely to conform
to the style guide.
jaredbeck added a commit that referenced this pull request Nov 8, 2015
Use class inst. var. instead of class var.
@jaredbeck jaredbeck merged commit f4818b6 into master Nov 8, 2015
@jaredbeck jaredbeck deleted the use_civ_not_cv branch November 8, 2015 17:36
@batter
Copy link
Collaborator

batter commented Nov 8, 2015

My only question about this is regarding thread safety. I sone more or less thread safe? I know we brought in request_store for thread safety concerns regarding the store, is that a concern here?

@jaredbeck
Copy link
Member Author

My only question about this is regarding thread safety.

That's an important question. This should have no impact on thread safety. Both class variables (CV) and class instance variables (CIV) are equally thread-unsafe. That's why we use RequestStore.

In a class, there's actually a small difference between CV and CIV (In short, CV are inherited/shared and CIV are not) but in a module there is no difference, because you can't inherit from a module. So, this change should have no effect.

Basically, the ruby style guide says to never use CV (https://github.com/bbatsov/ruby-style-guide#no-class-vars), so this change just brings us in conformance with the style guide.

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