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.

Sign up for GitHub

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

Jump to bottom

Some small refactorings #26

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Some small refactorings #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/docker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: Push Docker images to DockerHub and ECR

on:
push:
branches: [main, master]
branches: [main]

jobs:
multiple-registries:
Expand Down
20 changes: 7 additions & 13 deletions lib/snippet_extractor/extended/build_syntax_trie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,11 @@ def add_rule(trie, rule)
def get_word_from_rule(rule)
rule = rule.start_rule if rule.is_a? MultilineRule

!rule.whole_word? ? rule.word : " #{rule.word} "
rule.whole_word? ? " #{rule.word} " : rule.word
end

def set_next_node(node, word, rule)
if word.empty?
handle_rule_placement(node, rule)
return
end
return handle_rule_placement(node, rule) if word.empty?

handle_next_node(node, word, rule)
end
Expand All @@ -40,8 +37,8 @@ def handle_next_node(node, word, rule)
raise "Mapping conflict: Trying to map #{rule} which has a repeating character before any character"
end

if node.mapping.key? next_letter
unless node.mapping[next_letter].instance_of? next_node_type
if node.mapping.key?(next_letter)
unless node.mapping[next_letter].instance_of?(next_node_type)
raise "Mapping conflict: #{node.word} and #{rule} have conflicting repeating rule on char #{next_letter}"
end
else
Expand All @@ -59,10 +56,7 @@ def get_info_for_next_node(word)
end

def handle_rule_placement(node, rule)
if node.action.nil?
set_rule(node, rule)
return
end
return set_rule(node, rule) unless node.action

unless are_compatible?(node.action, rule)
raise "Mapping conflict: #{node.word} has action #{node.action}, but #{rule} tries to overwrite it"
Expand All @@ -73,8 +67,8 @@ def handle_rule_placement(node, rule)

def are_compatible?(action, rule)
case rule
when MultilineRule then multilines_compatible? action, rule
when SimpleRule then single_lines_compatible? action, rule
when MultilineRule then multilines_compatible?(action, rule)
when SimpleRule then single_lines_compatible?(action, rule)
end
end

Expand Down
203 changes: 120 additions & 83 deletions lib/snippet_extractor/extended/parse_code.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,52 +3,52 @@ module Extended
class ParseCode
include Mandate

STOP_AT_FIRST_LINE_OF_CODE = "stop_at_first_loc".freeze

def initialize(code, syntax_trie, arguments = [])
@code = code
@syntax_trie = syntax_trie
@arguments = arguments
@stop_at_first_loc = arguments.include?(STOP_AT_FIRST_LINE_OF_CODE)

@scan_index = 0

@current_line = ""
@current_line_has_skip = false
@current_action = nil
@queued_multiline = nil

@saved_lines = []
generate_new_line!
end

def call
action, skipped = try_match_first_word
execute_action(action, skipped) unless action.nil?
execute_action(action, skipped) if action

follow_strategy until is_finished?

save_last_line
save_last_line!

self.saved_lines
saved_lines.map(&:content)
end

protected
attr_accessor :code, :syntax_trie, :scan_index, :current_syntax_node, :current_line, :current_line_skipped,
:current_action, :queued_multiline, :saved_lines, :arguments, :current_line_has_skip
attr_accessor :code, :syntax_trie, :scan_index, :current_syntax_node, :current_line,
:current_action, :queued_multiline, :saved_lines

def try_match_first_word
try_match(syntax_trie.root.get_match_node(' ')) if syntax_trie.root.has_match?(' ')
return unless syntax_trie.root.has_match?(' ')

try_match(syntax_trie.root.get_match_node(' '))
end

def try_match(from_node = nil)
if self.arguments.include?(STOP_AT_FIRST_LINE_OF_CODE) && !(self.saved_lines.empty? && self.current_line.empty?)
return [nil, 0]
end
return if stop_at_first_loc? && !(saved_lines.empty? && current_line.empty?)

current_syntax_node = from_node || self.syntax_trie.root
current_syntax_node = from_node || syntax_trie.root
scan_lookahead = 0

while !is_finished?(scan_lookahead) && current_syntax_node.has_match?(scan_char(scan_lookahead))
current_syntax_node = current_syntax_node.get_match_node(scan_char(scan_lookahead))
until is_finished?(scan_lookahead)
node = scan_char!(scan_lookahead)
break unless current_syntax_node.has_match?(node)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Using this syntax, how about calling the method has_node_for?(node) or calling the node next_node instead? Is not possible at simple glance to know thatr scan_char brings the mapping to the next node, and not the actual node (which is brought by get_match_node


current_syntax_node = current_syntax_node.get_match_node(node)
scan_lookahead += 1
end

Expand All @@ -58,119 +58,156 @@ def try_match(from_node = nil)
def execute_action(action, skipped)
return if action.nil?

if action.instance_of? Multi
if action.instance_of?(Multi)
self.queued_multiline = action
execute_action(action.start_action, skipped)
return
end

save_if_newline_reached(skipped)
self.scan_index += skipped
self.current_line_has_skip = true
save_if_newline_reached!(skipped)
current_line.skip!

if action.instance_of?(Just) || (action.instance_of?(Line) && code[self.scan_index - 1].include?("\n"))
if action.instance_of?(Just) ||
(action.instance_of?(Line) && code[scan_index - 1] == "\n")
self.current_action = self.queued_multiline
self.queued_multiline = nil
else
self.current_action = action
end
end

def save_if_newline_reached(skipped = 1)
return unless code[self.scan_index, skipped].include?("\n")
def save_if_newline_reached!(size = 1)
if code[scan_index, size].include?("\n")
save_current_line!
generate_new_line!
end

save_current_line
self.current_line = ""
increment_scan_index!(size)
Copy link
Collaborator

@joshiraez joshiraez May 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are coupling the line saving with the progressing the scan index here. I would personally leave the increment_scan_index! where it was (line 68). Although in the current implementation, both go hand by hand (as both are actions that should be taken at the same time), the concepts are together by chance rather than by feature.

Personally I also found it a bit confusing. I guess it should work, but I don't expect the new line storage method to be the one progressing the pointer, but rather the scan/skip methods.

(See next comment) I think it would be mostly solved changing the name of the method to advance_index which is the important thing. The saving new line is a "byproduct" of reaching a newline while advancing the index, plus advancing the index is the main "action" in the parser, and currently is confusing to know when the index is advanced.

end

def follow_strategy
case self.current_action
when Line then line_strategy
when Multi then multi_strategy
case current_action
when Line then follow_line_strategy
when Multi then follow_multi_strategy
else
looking_for_match_strategy
end
end

def looking_for_match_strategy
action, skipped = try_match
if action.nil?
self.current_line = self.current_line + self.code[self.scan_index]
save_if_newline_reached
self.scan_index += 1
else
execute_action(action, skipped)
end
end

def line_strategy
if self.code[self.scan_index] == "\n"
def follow_line_strategy
if code[scan_index] == "\n"
self.current_action = self.queued_multiline
self.queued_multiline = nil
end

# We have to check for matches even though the line was skipped.
# The newline could be the blank space of the next whole word keyword.
if self.current_action.nil?
action, skipped = try_match
if action.nil?
self.current_line = self.current_line + self.code[self.scan_index]
save_if_newline_reached
self.scan_index += 1
else
execute_action(action, skipped)
end
else
save_if_newline_reached
self.scan_index += 1
end

return save_if_newline_reached! if current_action

looking_for_match_strategy
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks much better like this. Is right, that the special case is just a looking for match strategy (although I think I tried that and didnt woooork...? But I digress, this looks much cleaner if it's working).

Now that I'm seeing this save_if_newline_reached!, another way to put that if you want to do both the index progressing and the newline saving in one method, would be to call it advance_index, and, inside of advance index, leave the method just the same. I think it would be much more readable that way 🤔 just because, I "expect" to look where the index is advanced. Do I make sense?

end

def multi_strategy
action, skipped = try_match(self.current_action.syntax_trie.root)
if action.nil?
save_if_newline_reached
self.scan_index += 1
else
execute_action(action, skipped)
end
def follow_multi_strategy
action, skipped = try_match(current_action.syntax_trie.root)

return execute_action(action, skipped) if action

save_if_newline_reached!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. I think the idea of renaming it to advance_index makes a lot of sense when reading these methods.

end

def looking_for_match_strategy
action, skipped = try_match
return execute_action(action, skipped) if action

current_line.append(code[scan_index])
save_if_newline_reached!
end

def start_at_new_word
self.current_syntax_node = self.current_syntax_node.mapping[' '] if self.current_syntax_node.mapping.key? ' '
self.current_syntax_node = current_syntax_node.mapping[' '] if current_syntax_node.mapping.key?(' ')
end

def stop_at_first_loc?
!!@stop_at_first_loc
end

def is_finished?(lookahead = 0)
self.scan_index + lookahead >= self.code.length
scan_index + lookahead >= code.length
end

def scan_char(lookahead = 0)
character = self.code[self.scan_index + lookahead]
def scan_char!(lookahead = 0)
character = code[scan_index + lookahead]

character.strip.empty? ? ' ' : character.downcase
end

def save_current_line
if self.current_line.strip.empty?
unless self.current_line_has_skip || self.saved_lines.empty? || self.saved_lines.last.strip.empty?
self.current_line.strip!
add_newline_and_save_line
end
else
add_newline_and_save_line
end
self.current_line = ""
self.current_line_has_skip = false unless current_action.instance_of?(Multi) && queued_multiline.nil?
def save_current_line!
# If the current line is not empty, then save it and move on
add_newline_and_save_line! and return unless current_line.empty?(strip: true)

# The current line is empty. Sometimes we want to save it, sometimes we don't.
return if current_line.skip? || saved_lines.empty? || saved_lines.last.empty?(strip: true)

current_line.strip!
add_newline_and_save_line!
end

def generate_new_line!
self.current_line = CurrentLine.new(current_action.instance_of?(Multi) && queued_multiline.nil?)
end

def add_newline_and_save_line!
current_line.append("\n") unless current_line.ends_with_newline?
saved_lines.append(current_line)
end

def save_last_line!
saved_lines.append(current_line) unless current_line.empty?(strip: true)
end

def add_newline_and_save_line
self.current_line += "\n" if self.current_line[-1, 1] != "\n"
self.saved_lines.append(self.current_line)
def increment_scan_index!(size = 1)
self.scan_index += size
end

def save_last_line
self.saved_lines.append(self.current_line) unless self.current_line.strip.empty?
STOP_AT_FIRST_LINE_OF_CODE = "stop_at_first_loc".freeze
private_constant :STOP_AT_FIRST_LINE_OF_CODE

CurrentLine = Struct.new(:skip) do
def initialize(skip)
@content = ""
@skip = skip
end

def content
@content.freeze
end

def skip?
!!@skip
end

def skip!
@skip = true
end

def empty?(strip: false)
strip ? @content.strip.empty? : @content.empty?
end

def strip!
@content.strip!
end

def append(extra)
@content += extra
end

def ends_with_newline?
@content[-1, 1] == "\n"
end
end
private_constant :CurrentLine
end
end
end