Skip to content

Conversation

@GabrielLidenor
Copy link

This helps the us to understand the regex and make it easier for a future refactoring when we want to move some methods out of the base.rb file

Moved them into methods to make it easier to understand and easier to
move to another file in a future refactor
@mame
Copy link
Member

mame commented Oct 30, 2025

Hi, thanks for working on this.

I suspected this might not work, and it looks like the failing CI confirms that. The special variable $~ is local to the scope where the match operation occurs. If String#match is called inside a new sub-method, $~ won't be visible or set in the original calling method.

Regarding the new comments, I personally find that for developers who are comfortable with regular expressions, seeing the Regexp components directly is more concise and easier to understand.

What I think would be very valuable, though, is adding comments about the intent. For example, a comment explaining the specific edge cases that led to the decision to skip closing parens would be extremely helpful for future maintenance. Actually, I can't recall the exact reason right off the top of my head :-)

@GabrielLidenor
Copy link
Author

Perfect, I will be working on those changes you mentioned. Thanks for the fast response.

@GabrielLidenor
Copy link
Author

will open a new PR when I am ready

@GabrielLidenor GabrielLidenor deleted the gabriellidenor/refactor-regex branch November 4, 2025 20:44
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.

2 participants