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

route method regex explained #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Lucas-lufa
Copy link

@Lucas-lufa Lucas-lufa commented Sep 10, 2024

Put in comments to explain what the regex in the route method is doing

Summary by Sourcery

Add explanatory comments to the regex pattern in the route method to clarify its functionality.

Documentation:

  • Add comments to explain the regex pattern used in the route method, detailing the purpose of each component in the pattern.

Copy link

sourcery-ai bot commented Sep 10, 2024

Reviewer's Guide by Sourcery

This pull request adds comments to explain the regex pattern used in the route method. The changes focus on providing a detailed explanation of each component of the regex, improving code readability and understanding.

File-Level Changes

Change Details Files
Added comments explaining the regex pattern in the route method
  • Explained the purpose of forward slashes in the regex
  • Clarified the use of backslash to escape curly braces
  • Described the function of parentheses as capturing groups
  • Explained the meaning of the dot in the regex
  • Clarified the purpose of the plus sign
  • Described the function of the question mark for lazy matching
session-07/S07-Vanilla-PHP-MVC-Pt-03.md

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Lucas-lufa - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider consolidating the regex comments into a single, more comprehensive explanation of the regex's purpose in the routing mechanism, rather than explaining individual syntax elements.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟡 Documentation: 1 issue found

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

Comment on lines 426 to 431
// /is the start and end of regex pattern
// \escapes the curly brace, making it a regular character, it's looking for it.
// parenthesis is the capturing group
// . is any character that is not a newline
// + is one or more proceeding elements
// ? lazy making match as few characters as possible
Copy link

Choose a reason for hiding this comment

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

suggestion (documentation): Consider condensing the regex explanation comments

While the detailed explanation is helpful, it might be more concise to combine some of these comments. For example: '// / marks regex start/end, \ escapes {, (.+?) captures any chars (non-greedy)'

Suggested change
// /is the start and end of regex pattern
// \escapes the curly brace, making it a regular character, it's looking for it.
// parenthesis is the capturing group
// . is any character that is not a newline
// + is one or more proceeding elements
// ? lazy making match as few characters as possible
// / marks regex start/end, \ escapes {, (.+?) captures any chars (non-greedy)
// () is capturing group, . any char except newline, + one or more, ? lazy match

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.

1 participant