Extract shared Displayable concern from Mod and Tool models#72
Extract shared Displayable concern from Mod and Tool models#72
Conversation
Moves duplicated methods into a shared concern: - readme / details (memoized README fetching with fallback) - author_slug (parameterized author name) - updated_string / version_string (display formatting) - fetch_readme (HTTP fetch with error handling) Reduces ~50 lines of duplication between the two models. Closes #63
dyoung522
temporarily deployed
to
workflow-test
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
Extracts duplicated presentation- and README-fetching behavior from Mod and Tool into a shared Displayable concern to reduce duplication and keep formatting/fetch logic consistent across both models.
Changes:
- Added
Displayableconcern encapsulatingreadme/details,author_slug,updated_string,version_string, andfetch_readme. - Updated
ModandToolto includeDisplayableand removed their duplicated implementations. - Fixed spelling in
Modcomments (“Determins” → “Determines”).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/models/tool.rb | Replaces duplicated display/README methods with include Displayable. |
| app/models/mod.rb | Replaces duplicated display/README methods with include Displayable (plus comment typo fixes). |
| app/models/concerns/displayable.rb | Introduces shared concern for README fetching and display helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/models/concerns/displayable.rb
Outdated
|
|
||
| def fetch_readme | ||
| # We strip out the first # line of the README, as it's usually a title | ||
| Net::HTTP.get(raw_uri(readme_url)).gsub(/^#\s+.*$/, "").strip |
There was a problem hiding this comment.
In fetch_readme, the regex gsub(/^#\s+.*$/, "") will remove every Markdown heading line in the README (because ^/$ match line boundaries), not just the first title line as the comment says. If the goal is to strip only the leading H1/title, consider using sub anchored to the start of the string (e.g., \A#...) or otherwise limiting the replacement to the first match so subsequent headings are preserved.
| Net::HTTP.get(raw_uri(readme_url)).gsub(/^#\s+.*$/, "").strip | |
| Net::HTTP.get(raw_uri(readme_url)).sub(/\A#\s+.*\n?/, "").strip |
The original gsub(/^#\s+.*$/) removed ALL markdown heading lines. Changed to sub(/\A#\s+.*\n?/) to strip only the first H1 at the start of the document, preserving other headings in the README. Addresses CoPilot review feedback.
dyoung522
temporarily deployed
to
workflow-test
GitHub Actions
Inactive
Extracts duplicated methods from
ModandToolinto a newDisplayableconcern:readme/details— memoized README fetching with description fallbackauthor_slug— parameterized author nameupdated_string/version_string— display formattingfetch_readme— HTTP fetch with comprehensive error handlingBefore: ~50 lines duplicated across both models
After: Single source of truth in
app/models/concerns/displayable.rb230 specs passing, RuboCop clean.
Closes #63