Copilot commented on code in PR #7167:
URL: https://github.com/apache/hbase/pull/7167#discussion_r2225849488
##########
hbase-shell/src/main/ruby/irb/hirb.rb:
##########
@@ -23,6 +23,70 @@ module IRB
# Subclass of IRB so can intercept methods
class HIRB < Irb
+ # Method for checking the variable name conflict with the hbase command
name
+ def check_variable_assignment_conflict(line)
+ # checks when a single variable is defined at a time (eg a = 10) or
multiple variable assignment (e.g., a,b,c = 1,2,3)
+ single_match =
line.strip.match(/^([a-zA-Z_][a-zA-Z0-9_]*)\s*=\s*(.+)/)
+ multiple_match =
line.strip.match(/^([a-zA-Z_][a-zA-Z0-9_]*(?:\s*,\s*[a-zA-Z_][a-zA-Z0-9_]*)+)\s*=\s*(.+)/)
+
+ # Match comma-based expressions that may result in nil assignment to
a variable
+ comma_expression =
line.strip.match(/^([a-zA-Z_][a-zA-Z0-9_]*)\s*,\s*(.+)/)
+
+ variable_names = []
+
+ if multiple_match
+ # Extract all variable names during multiple assignment
+ left_side = multiple_match[1]
+ variable_names = left_side.split(',').map(&:strip)
+ elsif single_match
+ # Extracting variable name for single assignment
+ variable_names = [single_match[1]]
+ elsif comma_expression
+ # Handle comma expressions that create variables (like
list_namespace,'ns.*')
+ variable_names = [comma_expression[1]]
+ else
+ # if no assignment pattern found
+ return false
+ end
+
+ # return false if variable_names is empty
+ return false if variable_names.empty?
+
+ receiver = @context.workspace.binding.receiver
+ conflicting_variables = []
+
+ # Checking variable names for conflicts with the HBase shell command
names
+ variable_names.each do |variable_name|
+ conflicts = []
+
+ # if variable is defined as the command name adding it to
conflicts list
+ if defined?(::Shell) && ::Shell.respond_to?(:commands) &&
::Shell.commands.key?(variable_name)
+ conflicts << "HBase command"
+ end
+
+ # If this variable has conflicts, add it to the list
+ unless conflicts.empty?
+ conflicting_variables << {
+ name: variable_name,
+ conflicts: conflicts
+ }
+ end
+ end
+
+ # If any conflicts found, block the entire assignment
+ unless conflicting_variables.empty?
+ puts "ERROR: Cannot create variable assignment"
+ conflicting_variables.each do |var_info|
+ puts "'#{var_info[:name]}' conflicts with:
#{var_info[:conflicts].join(', ')}"
+ end
+ puts "This could cause unexpected behavior or make commands
unusable"
+ puts "Please use different variable names"
+ puts
+ return true # Block assignment
+ end
+ return false # Allow assignment
+ end
Review Comment:
The method has inconsistent indentation. It should use 6 spaces to match the
class indentation level, not 8 spaces.
```suggestion
# Method for checking the variable name conflict with the hbase command
name
def check_variable_assignment_conflict(line)
# checks when a single variable is defined at a time (eg a = 10) or
multiple variable assignment (e.g., a,b,c = 1,2,3)
single_match =
line.strip.match(/^([a-zA-Z_][a-zA-Z0-9_]*)\s*=\s*(.+)/)
multiple_match =
line.strip.match(/^([a-zA-Z_][a-zA-Z0-9_]*(?:\s*,\s*[a-zA-Z_][a-zA-Z0-9_]*)+)\s*=\s*(.+)/)
# Match comma-based expressions that may result in nil assignment to
a variable
comma_expression =
line.strip.match(/^([a-zA-Z_][a-zA-Z0-9_]*)\s*,\s*(.+)/)
variable_names = []
if multiple_match
# Extract all variable names during multiple assignment
left_side = multiple_match[1]
variable_names = left_side.split(',').map(&:strip)
elsif single_match
# Extracting variable name for single assignment
variable_names = [single_match[1]]
elsif comma_expression
# Handle comma expressions that create variables (like
list_namespace,'ns.*')
variable_names = [comma_expression[1]]
else
# if no assignment pattern found
return false
end
# return false if variable_names is empty
return false if variable_names.empty?
receiver = @context.workspace.binding.receiver
conflicting_variables = []
# Checking variable names for conflicts with the HBase shell command
names
variable_names.each do |variable_name|
conflicts = []
# if variable is defined as the command name adding it to
conflicts list
if defined?(::Shell) && ::Shell.respond_to?(:commands) &&
::Shell.commands.key?(variable_name)
conflicts << "HBase command"
end
# If this variable has conflicts, add it to the list
unless conflicts.empty?
conflicting_variables << {
name: variable_name,
conflicts: conflicts
}
end
end
# If any conflicts found, block the entire assignment
unless conflicting_variables.empty?
puts "ERROR: Cannot create variable assignment"
conflicting_variables.each do |var_info|
puts "'#{var_info[:name]}' conflicts with:
#{var_info[:conflicts].join(', ')}"
end
puts "This could cause unexpected behavior or make commands
unusable"
puts "Please use different variable names"
puts
return true # Block assignment
end
return false # Allow assignment
end
```
##########
hbase-shell/src/main/ruby/irb/hirb.rb:
##########
@@ -23,6 +23,70 @@ module IRB
# Subclass of IRB so can intercept methods
class HIRB < Irb
+ # Method for checking the variable name conflict with the hbase command
name
+ def check_variable_assignment_conflict(line)
+ # checks when a single variable is defined at a time (eg a = 10) or
multiple variable assignment (e.g., a,b,c = 1,2,3)
+ single_match =
line.strip.match(/^([a-zA-Z_][a-zA-Z0-9_]*)\s*=\s*(.+)/)
+ multiple_match =
line.strip.match(/^([a-zA-Z_][a-zA-Z0-9_]*(?:\s*,\s*[a-zA-Z_][a-zA-Z0-9_]*)+)\s*=\s*(.+)/)
+
+ # Match comma-based expressions that may result in nil assignment to
a variable
+ comma_expression =
line.strip.match(/^([a-zA-Z_][a-zA-Z0-9_]*)\s*,\s*(.+)/)
+
+ variable_names = []
+
+ if multiple_match
+ # Extract all variable names during multiple assignment
+ left_side = multiple_match[1]
+ variable_names = left_side.split(',').map(&:strip)
+ elsif single_match
+ # Extracting variable name for single assignment
+ variable_names = [single_match[1]]
+ elsif comma_expression
+ # Handle comma expressions that create variables (like
list_namespace,'ns.*')
+ variable_names = [comma_expression[1]]
+ else
+ # if no assignment pattern found
+ return false
+ end
+
+ # return false if variable_names is empty
+ return false if variable_names.empty?
+
+ receiver = @context.workspace.binding.receiver
+ conflicting_variables = []
+
+ # Checking variable names for conflicts with the HBase shell command
names
+ variable_names.each do |variable_name|
+ conflicts = []
+
+ # if variable is defined as the command name adding it to
conflicts list
+ if defined?(::Shell) && ::Shell.respond_to?(:commands) &&
::Shell.commands.key?(variable_name)
+ conflicts << "HBase command"
+ end
+
+ # If this variable has conflicts, add it to the list
+ unless conflicts.empty?
+ conflicting_variables << {
+ name: variable_name,
+ conflicts: conflicts
Review Comment:
The variable 'conflicts' is initialized inside the loop but only used to
check if ::Shell.commands.key?(variable_name). This can be simplified by
directly checking the condition without the intermediate array.
```suggestion
# Check if the variable name conflicts with HBase shell command
names
if defined?(::Shell) && ::Shell.respond_to?(:commands) &&
::Shell.commands.key?(variable_name)
conflicting_variables << {
name: variable_name,
conflicts: ["HBase command"]
```
##########
hbase-shell/src/main/ruby/irb/hirb.rb:
##########
@@ -112,6 +176,7 @@ def eval_input
signal_status(:IN_EVAL) do
begin
line.untaint
+ next if check_variable_assignment_conflict(line)
Review Comment:
The indentation is inconsistent with the surrounding code. This line should
have 14 spaces to align with the other statements in the begin block.
```suggestion
next if check_variable_assignment_conflict(line)
```
##########
hbase-shell/src/main/ruby/irb/hirb.rb:
##########
@@ -112,6 +176,7 @@ def eval_input
signal_status(:IN_EVAL) do
begin
line.untaint
Review Comment:
The 'untaint' method has been deprecated and removed in Ruby 3.2+. This
could cause compatibility issues with newer Ruby versions.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
