Skip to content
Projects
Groups
Snippets
Help
Loading...
Help
Support
Keyboard shortcuts
?
Submit feedback
Contribute to GitLab
Sign in / Register
Toggle navigation
G
gitlab-ce
Project overview
Project overview
Details
Activity
Releases
Repository
Repository
Files
Commits
Branches
Tags
Contributors
Graph
Compare
Issues
0
Issues
0
List
Boards
Labels
Milestones
Merge Requests
0
Merge Requests
0
Analytics
Analytics
Repository
Value Stream
Wiki
Wiki
Snippets
Snippets
Members
Members
Collapse sidebar
Close sidebar
Activity
Graph
Create a new issue
Commits
Issue Boards
Open sidebar
Léo-Paul Géneau
gitlab-ce
Commits
45568bed
Commit
45568bed
authored
Nov 21, 2017
by
Lin Jen-Shin
Browse files
Options
Browse Files
Download
Email Patches
Plain Diff
Updates based on feedback
parent
ffec300b
Changes
7
Hide whitespace changes
Inline
Side-by-side
Showing
7 changed files
with
108 additions
and
121 deletions
+108
-121
app/models/concerns/spammable.rb
app/models/concerns/spammable.rb
+1
-4
doc/development/README.md
doc/development/README.md
+1
-0
doc/development/module_with_instance_variables.md
doc/development/module_with_instance_variables.md
+12
-13
features/support/env.rb
features/support/env.rb
+2
-2
lib/gitlab/ci/config/entry/configurable.rb
lib/gitlab/ci/config/entry/configurable.rb
+3
-3
rubocop/cop/module_with_instance_variables.rb
rubocop/cop/module_with_instance_variables.rb
+2
-4
spec/rubocop/cop/module_with_instance_variables_spec.rb
spec/rubocop/cop/module_with_instance_variables_spec.rb
+87
-95
No files found.
app/models/concerns/spammable.rb
View file @
45568bed
...
...
@@ -12,6 +12,7 @@ module Spammable
attr_accessor
:spam
attr_accessor
:spam_log
alias_method
:spam?
,
:spam
after_validation
:check_for_spam
,
on:
[
:create
,
:update
]
...
...
@@ -34,10 +35,6 @@ module Spammable
end
end
def
spam?
spam
end
def
check_for_spam
error_msg
=
if
Gitlab
::
Recaptcha
.
enabled?
"Your
#{
spammable_entity_type
}
has been recognized as spam. "
\
...
...
doc/development/README.md
View file @
45568bed
...
...
@@ -36,6 +36,7 @@ comments: false
-
[
`Gemfile` guidelines
](
gemfile.md
)
-
[
Sidekiq debugging
](
sidekiq_debugging.md
)
-
[
Gotchas
](
gotchas.md
)
to avoid
-
[
Avoid modules with instance variables
](
module_with_instance_variables.md
)
if possible
-
[
Issue and merge requests state models
](
object_state_models.md
)
-
[
How to dump production data to staging
](
db_dump.md
)
-
[
Working with the GitHub importer
](
github_importer.md
)
...
...
doc/development/module_with_instance_variables.md
View file @
45568bed
...
...
@@ -55,10 +55,9 @@ they communicate in a clear way, rather than implicit dependencies.
### Acceptable use
However, it's not all that bad when using instance variables in a module,
as long as it's contained in the same module, that is no other modules or
objects are touching them. If that's the case, then it would be an acceptable
use.
However, it's not always bad to use instance variables in a module,
as long as it's contained in the same module; that is, no other modules or
objects are touching them, then it would be an acceptable use.
We especially allow the case where a single instance variable is used with
`||=`
to setup the value. This would look like:
...
...
@@ -93,7 +92,7 @@ module Gitlab
end
```
It's still offending because it's not just
`||=`
, but
W
e could split this
It's still offending because it's not just
`||=`
, but
w
e could split this
method into two:
```
ruby
...
...
@@ -135,7 +134,7 @@ end
```
There are several implicit dependencies here. First,
`params`
should be
defined before us
ing
. Second,
`filter_spam_check_params`
should be called
defined before us
e
. Second,
`filter_spam_check_params`
should be called
before
`spam_check`
. These are all implicit and the includer could be using
those instance variables without awareness.
...
...
@@ -175,18 +174,18 @@ end
```
This way, all those instance variables are isolated in
`SpamCheckService`
rather than wh
o ever include
the module, and those modules which were also
included, making it much easier to track down
the issues if there's any
,
and
it also reduces
the chance of having name conflicts.
rather than wh
atever includes
the module, and those modules which were also
included, making it much easier to track down
any issues
,
and
reducing
the chance of having name conflicts.
### Things we might need to ignore right now
Since the way how
Rails helpers and mailers work, we might not be able to
Because of the way
Rails helpers and mailers work, we might not be able to
avoid the use of instance variables there. For those cases, we could ignore
them at the moment. At least we're not going to share those modules with
other random objects, so they're still some
how
isolated.
other random objects, so they're still some
what
isolated.
### Instance variables in
the
views
### Instance variables in views
They're terrible, because they're also shared between different controllers,
and it's very hard to track where those instance variables were set when we
...
...
@@ -210,5 +209,5 @@ And in the partial:
-
project
=
local_assigns
.
fetch
(
:project
)
```
This way it's
very clea
r where those values were coming from. In the future,
This way it's
cleare
r where those values were coming from. In the future,
we should also forbid the use of instance variables in partials.
features/support/env.rb
View file @
45568bed
...
...
@@ -42,11 +42,11 @@ module StdoutReporterWithScenarioLocation
# Override the standard reporter to show filename and line number next to each
# scenario for easy, focused re-runs
def
before_scenario_run
(
scenario
,
step_definitions
=
nil
)
max_step_name_length
=
scenario
.
steps
.
map
(
&
:name
).
map
(
&
:length
).
max
if
scenario
.
steps
.
any?
@max_step_name_length
=
scenario
.
steps
.
map
(
&
:name
).
map
(
&
:length
).
max
if
scenario
.
steps
.
any?
# rubocop:disable Cop/ModuleWithInstanceVariables
name
=
scenario
.
name
# This number has no significance, it's just to line things up
max_length
=
max_step_name_length
+
19
max_length
=
@max_step_name_length
+
19
# rubocop:disable Cop/ModuleWithInstanceVariables
out
.
puts
"
\n
#{
'Scenario:'
.
green
}
#{
name
.
light_green
.
ljust
(
max_length
)
}
"
\
" #
#{
scenario
.
feature
.
filename
}
:
#{
scenario
.
line
}
"
end
...
...
lib/gitlab/ci/config/entry/configurable.rb
View file @
45568bed
...
...
@@ -59,13 +59,13 @@ module Gitlab
def
helpers
(
*
nodes
)
nodes
.
each
do
|
symbol
|
define_method
(
"
#{
symbol
}
_defined?"
)
do
@
entries
[
symbol
]
&
.
specified?
entries
[
symbol
]
&
.
specified?
end
define_method
(
"
#{
symbol
}
_value"
)
do
return
unless
@entries
[
symbol
]
&&
@
entries
[
symbol
].
valid?
return
unless
entries
[
symbol
]
&&
entries
[
symbol
].
valid?
@
entries
[
symbol
].
value
entries
[
symbol
].
value
end
end
end
...
...
rubocop/cop/module_with_instance_variables.rb
View file @
45568bed
...
...
@@ -5,7 +5,7 @@ module RuboCop
Do not use instance variables in a module. Please read this
for the rationale behind it:
doc/development/module_with_instance_variables.md
https://docs.gitlab.com/ee/development/module_with_instance_variables.html
If you think the use for this is fine, please just add:
# rubocop:disable Cop/ModuleWithInstanceVariables
...
...
@@ -56,9 +56,7 @@ module RuboCop
add_offense
(
offense
,
:expression
)
end
# We allow initialize method and single ivar
elsif
initialize_method?
(
definition
)
||
single_ivar?
(
definition
)
next
else
elsif
!
initialize_method?
(
definition
)
&&
!
single_ivar?
(
definition
)
definition
.
each_descendant
(
:ivar
,
:ivasgn
)
do
|
offense
|
add_offense
(
offense
,
:expression
)
end
...
...
spec/rubocop/cop/module_with_instance_variables_spec.rb
View file @
45568bed
...
...
@@ -8,7 +8,9 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do
subject
(
:cop
)
{
described_class
.
new
}
shared_examples
(
'registering offense'
)
do
shared_examples
(
'registering offense'
)
do
|
options
|
let
(
:offending_lines
)
{
options
[
:offending_lines
]
}
it
'registers an offense when instance variable is used in a module'
do
inspect_source
(
cop
,
source
)
...
...
@@ -28,63 +30,57 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do
end
context
'when source is a regular module'
do
let
(
:source
)
do
<<~
RUBY
module M
def f
@f = true
it_behaves_like
'registering offense'
,
offending_lines:
[
3
]
do
let
(
:source
)
do
<<~
RUBY
module M
def f
@f = true
end
end
end
RUBY
RUBY
end
end
let
(
:offending_lines
)
{
[
3
]
}
it_behaves_like
'registering offense'
end
context
'when source is a nested module'
do
let
(
:source
)
do
<<~
RUBY
module N
module M
def f
@f = true
it_behaves_like
'registering offense'
,
offending_lines:
[
4
]
do
let
(
:source
)
do
<<~
RUBY
module N
module M
def f
@f = true
end
end
end
end
RUBY
RUBY
end
end
let
(
:offending_lines
)
{
[
4
]
}
it_behaves_like
'registering offense'
end
context
'when source is a nested module with multiple offenses'
do
let
(
:source
)
do
<<~
RUBY
module N
module M
def f
@f = true
end
def g
true
end
def h
@h = true
it_behaves_like
'registering offense'
,
offending_lines:
[
4
,
12
]
do
let
(
:source
)
do
<<~
RUBY
module N
module M
def f
@f = true
end
def g
true
end
def h
@h = true
end
end
end
end
RUBY
RUBY
end
end
let
(
:offending_lines
)
{
[
4
,
12
]
}
it_behaves_like
'registering offense'
end
context
'with regular ivar assignment'
do
...
...
@@ -124,78 +120,74 @@ describe RuboCop::Cop::ModuleWithInstanceVariables do
end
context
'when source is using simple or ivar assignment'
do
let
(
:source
)
do
<<~
RUBY
module M
def f
@f ||= true
it_behaves_like
'not registering offense'
do
let
(
:source
)
do
<<~
RUBY
module M
def f
@f ||= true
end
end
end
RUBY
RUBY
end
end
it_behaves_like
'not registering offense'
end
context
'when source is using simple ivar'
do
let
(
:source
)
do
<<~
RUBY
module M
def f?
@f
it_behaves_like
'not registering offense'
do
let
(
:source
)
do
<<~
RUBY
module M
def f?
@f
end
end
end
RUBY
RUBY
end
end
it_behaves_like
'not registering offense'
end
context
'when source is defining initialize'
do
let
(
:source
)
do
<<~
RUBY
module M
def initialize
@a = 1
@b = 2
it_behaves_like
'not registering offense'
do
let
(
:source
)
do
<<~
RUBY
module M
def initialize
@a = 1
@b = 2
end
end
end
RUBY
RUBY
end
end
it_behaves_like
'not registering offense'
end
context
'when source is using simple or ivar assignment with other ivar'
do
let
(
:source
)
do
<<~
RUBY
module M
def f
@f ||= g(@g)
it_behaves_like
'registering offense'
,
offending_lines:
[
3
]
do
let
(
:source
)
do
<<~
RUBY
module M
def f
@f ||= g(@g)
end
end
end
RUBY
RUBY
end
end
let
(
:offending_lines
)
{
[
3
]
}
it_behaves_like
'registering offense'
end
context
'when source is using or ivar assignment with something else'
do
let
(
:source
)
do
<<~
RUBY
module M
def f
@f ||= true
@f.to_s
it_behaves_like
'registering offense'
,
offending_lines:
[
3
,
4
]
do
let
(
:source
)
do
<<~
RUBY
module M
def f
@f ||= true
@f.to_s
end
end
end
RUBY
RUBY
end
end
let
(
:offending_lines
)
{
[
3
,
4
]
}
it_behaves_like
'registering offense'
end
end
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment