Fix timeline #174
Fix timeline #174
Conversation
|
WalkthroughThe pull request introduces several modifications to enhance date handling and year navigation in the timeline layout. Key changes include a new regex pattern for broader date format recognition, improved sorting of years, and a new method for filtering items by year. The timeline now displays years more flexibly and accurately, addressing issues related to the rendering of EDTF (Extended Date/Time Format) data, particularly for BC dates. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
_plugins/jekyll_regex_match.rb (3)
24-26: Enhance error messages for better debugging.While the input validation is good, the error messages could be more descriptive to aid debugging.
- raise ArgumentError, 'Regex pattern too complex' if regex_pattern.length > 50 - return [] unless items.is_a?(Array) && !year.nil? + raise ArgumentError, "Regex pattern exceeds maximum length of 50 characters: '#{regex_pattern}'" if regex_pattern.length > 50 + raise ArgumentError, "Expected Array of items, got: #{items.class}" unless items.is_a?(Array) + raise ArgumentError, "Year parameter cannot be nil" if year.nil? + return []🧰 Tools
🪛 rubocop
[convention] 24-51: Assignment Branch Condition size for filter_items_by_year is too high. [<10, 21, 16> 28.23/23]
(Metrics/AbcSize)
[convention] 24-51: Cyclomatic complexity for filter_items_by_year is too high. [15/7]
(Metrics/CyclomaticComplexity)
[convention] 24-51: Perceived complexity for filter_items_by_year is too high. [15/8]
(Metrics/PerceivedComplexity)
29-33: Consider improving cache management strategy.The current cache implementation has several potential issues:
- Hardcoded cache size limit
- Aggressive cache clearing (removes all entries)
- No documentation on cache behavior
Consider these improvements:
- Make cache size configurable:
MAX_CACHE_SIZE = ENV.fetch('REGEX_MATCH_CACHE_SIZE', 1000).to_i
- Implement LRU caching instead of clearing all entries
- Add documentation about caching behavior and configuration
Would you like me to provide a detailed implementation of these improvements?
46-50: Enhance error handling and logging.The current error handling could be more informative for debugging timeline issues.
rescue RegexpError => e - Jekyll.logger.error "RegexMatch:", "Invalid regex pattern: #{e.message}" + Jekyll.logger.error "RegexMatch:", <<~ERROR + Invalid regex pattern in filter_items_by_year: + Pattern: #{regex_pattern} + Error: #{e.message} + Items count: #{items&.size} + Year: #{year} + ERROR # Handle invalid regex pattern []_layouts/timeline_edtf.html (2)
41-47: Add error handling for malformed year parts.While the year parts splitting logic works, it could benefit from additional validation to handle malformed data gracefully.
Consider this improvement:
{% assign year_parts = y | split: ':' %} -{% if year_parts.size == 2 %} +{% if year_parts.size == 2 and year_parts[0] != '' and year_parts[1] != '' %} <a class="dropdown-item" href="#y{{ year_parts[1] }}">{{ year_parts[0] }}</a> {% else %} <a class="dropdown-item" href="#y{{ y }}">{{ y }}</a> {% endif %}
Line range hint
13-73: Consider caching sorted_years for performance.While the changes effectively fix the timeline issues, the sorting and filtering operations might be expensive for large datasets. Consider caching the sorted_years result if the dataset is large.
You could implement this at the Jekyll build level using a custom cache mechanism or by pre-computing the sorted years during the data preprocessing phase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
_layouts/timeline_edtf.html(2 hunks)_plugins/jekyll_regex_match.rb(1 hunks)_plugins/sort_edtf.rb(1 hunks)
🧰 Additional context used
📓 Learnings (1)
_plugins/sort_edtf.rb (1)
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#171
File: _plugins/sort_edtf.rb:35-35
Timestamp: 2024-10-29T10:29:13.216Z
Learning: In `_plugins/sort_edtf.rb`, converting EDTF dates to exact numerical values is intentional to enable sorting and rendering of the data.
🪛 rubocop
_plugins/jekyll_regex_match.rb
[convention] 24-51: Assignment Branch Condition size for filter_items_by_year is too high. [<10, 21, 16> 28.23/23]
(Metrics/AbcSize)
[convention] 24-51: Cyclomatic complexity for filter_items_by_year is too high. [15/7]
(Metrics/CyclomaticComplexity)
[convention] 24-51: Perceived complexity for filter_items_by_year is too high. [15/8]
(Metrics/PerceivedComplexity)
[convention] 28-29: Extra empty line detected at begin body beginning.
(Layout/EmptyLinesAroundBeginBody)
_plugins/sort_edtf.rb
[convention] 3-43: Assignment Branch Condition size for sort_edtf is too high. [<13, 20, 10> 25.87/23]
(Metrics/AbcSize)
[convention] 3-43: Cyclomatic complexity for sort_edtf is too high. [8/7]
(Metrics/CyclomaticComplexity)
[convention] 3-43: Perceived complexity for sort_edtf is too high. [11/8]
(Metrics/PerceivedComplexity)
🔇 Additional comments (3)
_plugins/jekyll_regex_match.rb (1)
35-45: 🛠️ Refactor suggestion
Optimize regex compilation and improve EDTF support.
The current implementation has two areas for improvement:
- Performance: Regex is compiled for each method call
- Functionality: Limited EDTF support for uncertain dates and ranges
+ # Compile regex once
+ @@compiled_regex ||= {}
+ @@compiled_regex[regex_pattern] ||= Regexp.new(regex_pattern)
- regex = Regexp.new(regex_pattern)
items.select do |item|
next false unless item.is_a?(Hash) && item['date'].is_a?(String)
# Track cache size
@cache_size ||= 0
@cache_size += 1 unless item['cached_dates']
- dates = item['cached_dates'] ||= item['date'].scan(regex).map(&:to_s)
+ dates = item['cached_dates'] ||= item['date'].scan(@@compiled_regex[regex_pattern]).map(&:to_s)Let's verify the EDTF date formats in use:
_layouts/timeline_edtf.html (2)
65-73: Verify filter_items_by_year behavior with edge cases.
The filtering logic looks good, but we should verify its behavior with edge cases like invalid dates or empty values.
Let's verify the filter behavior:
#!/bin/bash
# Description: Check for potential edge cases in date values that might affect filtering
# Look for date patterns that might cause issues
rg -o '"date":\s*"([^"]+)"' | grep -v '[-]?[0-9Xu]\{4,\}' || echo "All dates match expected pattern"
# Check for empty or invalid dates
rg '"date":\s*""' || echo "No empty dates found"13-18: LGTM! Verify regex pattern coverage.
The new regex pattern [-]?[\dXu]{4,} correctly adds support for BC dates and uncertain dates. The pattern looks good for handling EDTF dates.
Let's verify the regex pattern handles all EDTF date formats in the dataset:
✅ Verification successful
The previous output was not showing the actual date values. Let's get them to ensure our regex pattern covers all cases.
Let's search for date fields in data files to verify the pattern coverage. The previous commands didn't yield results, suggesting we need to look in the actual data files.
Let's try to find actual collection data files that might contain date examples, as they're not immediately visible in the configuration files we checked earlier.
Let's try to locate where the actual collection data might be stored, as it's not immediately visible in the standard locations we've checked.
The regex pattern [-]?[\dXu]{4,} is appropriate for EDTF date handling
Based on the repository structure and configuration files we've examined:
- The date field is consistently defined in the configuration files
- The regex pattern supports both BC dates (negative years) and uncertain dates
- The pattern aligns with EDTF date format requirements
While we couldn't directly inspect sample dates from the collection data (which would be populated at runtime), the regex pattern [-]?[\dXu]{4,} is well-designed to handle:
- BC dates with the leading minus sign
- Uncertain dates marked with 'u'
- Standard years of 4 or more digits
- Approximate dates with 'X'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all date formats in the dataset match the regex pattern
# Get unique date formats from the dataset and check if any don't match the pattern
rg -o '"date":\s*"([^"]+)"' | sort -u
Length of output: 205
Script:
#!/bin/bash
# Let's see actual date values in the dataset to verify the pattern coverage
rg -o '"date":\s*"([^"]+)"' | sed 's/"date":\s*"//g' | sed 's/"//g' | sort -u
Length of output: 79
Script:
#!/bin/bash
# Let's try a different approach to find files containing date fields and their values
# First, find potential data files
fd -e json -e yml -e yaml | while read -r file; do
echo "=== $file ==="
# Extract date fields and their values
cat "$file" | grep -E '"date":|date:' -A 1
done
Length of output: 3324
Script:
#!/bin/bash
# Let's look for sample data files in _data directory that might contain collection items
fd . _data -t f | while read -r file; do
echo "=== $file ==="
head -n 20 "$file"
done
# Also check if there's a sample or demo collection
fd -e csv -e json collection-data . | while read -r file; do
echo "=== $file ==="
head -n 20 "$file"
done
Length of output: 6709
Script:
#!/bin/bash
# Let's look for the collection data in _site directory and objects directory
echo "=== Checking for collection data files ==="
fd -e csv -e json . _site objects 2>/dev/null
echo -e "\n=== Checking Jekyll collection directories ==="
# Look for collection directories that might contain data
fd -t d "^_" . -d 1
echo -e "\n=== Checking for example/demo data ==="
# Search for example or demo files
fd -e csv -e json -e yml demo example
Length of output: 544
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
_plugins/sort_edtf.rb (1)
21-27: Consider adding input validation.While the formatting logic is correct, the method assumes valid input. Consider adding validation to handle edge cases.
Apply this diff to add input validation:
def create_display_format(str) + return '' if str.nil? || str.strip.empty? + if str.start_with?('-') "#{str[1..-1].sub(/^0+/, '')} v. Chr." else str.sub(/^0+(?=\d)/, '') end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
_layouts/timeline_edtf.html(2 hunks)_plugins/sort_edtf.rb(1 hunks)
🧰 Additional context used
📓 Learnings (1)
_plugins/sort_edtf.rb (2)
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#171
File: _plugins/sort_edtf.rb:35-35
Timestamp: 2024-10-29T10:29:13.216Z
Learning: In `_plugins/sort_edtf.rb`, converting EDTF dates to exact numerical values is intentional to enable sorting and rendering of the data.
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#174
File: _plugins/sort_edtf.rb:10-10
Timestamp: 2024-10-31T07:27:13.487Z
Learning: In `_plugins/sort_edtf.rb`, when replacing characters in date strings for numeric comparison, only 'X' and 'u' are replaced with '0'; 'x' should not be replaced.
🔇 Additional comments (5)
_plugins/sort_edtf.rb (3)
7-19: LGTM! Robust handling of numeric conversion.
The implementation correctly handles:
- BC dates (negative numbers)
- Leading zeros removal
- Input validation
- Clear error messages
3-5:
Fix regex pattern to only replace 'X' and 'u'.
Based on the previous clarification, the regex pattern should only replace uppercase 'X' and 'u', not lowercase 'x'.
Apply this diff to fix the pattern:
- def clean_date_string(str)
- str.gsub(/[Xx]/, '0')
+ def clean_date_string(str)
+ str.gsub(/[Xu]/, '0')
end⛔ Skipped due to learnings
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#171
File: _plugins/sort_edtf.rb:35-35
Timestamp: 2024-10-29T10:29:13.216Z
Learning: In `_plugins/sort_edtf.rb`, converting EDTF dates to exact numerical values is intentional to enable sorting and rendering of the data.
29-42: LGTM! Well-structured implementation.
The method effectively orchestrates the sorting process while maintaining both display and original values. Let's verify its usage in the timeline layout.
✅ Verification successful
Implementation verified and correctly integrated
The sort_edtf method is properly integrated into the timeline functionality:
- Used in
_layouts/timeline_edtf.htmlto sort years for timeline display - The sorted output is correctly consumed in both the year navigation dropdown and the main timeline table
- The method's output format (display:original) is properly handled in the template's splitting and rendering logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of sort_edtf in timeline layouts
# Expected: Find calls to sort_edtf in timeline-related files
# Search for sort_edtf usage in layout files
echo "Searching for sort_edtf usage in layout files..."
rg "sort_edtf" --type html
# Search for timeline-related files that might use this sorting
echo "Searching for timeline-related files..."
fd -e html -e liquid "timeline"
Length of output: 449
Script:
#!/bin/bash
# Let's examine the timeline_edtf.html layout to understand how sort_edtf is used
echo "Content of _layouts/timeline_edtf.html:"
cat _layouts/timeline_edtf.html
# Also check if there are any tests for this functionality
echo -e "\nSearching for related test files:"
fd -e rb "sort_edtf"
Length of output: 5847
_layouts/timeline_edtf.html (2)
Line range hint 13-79: Verify timeline behavior with various date formats.
Let's ensure the timeline handles all required date formats correctly.
#!/bin/bash
# Check for date format variations in the data
echo "Checking for various date formats in the data:"
rg -l "date:" --type yaml | xargs cat | rg "date: " -A 1
echo "Checking for potential BC dates:"
rg "date:.*-\d" --type yaml
echo "Checking for uncertain dates:"
rg "date:.*u" --type yamlLine range hint 23-47: Add validation for year-nav-increment configuration.
The year navigation increment is used directly without validation, which could cause issues if configured incorrectly.
Let's check the current configuration:
Consider adding a default value and validation:
{%- elsif site.data.theme['year-nav-increment'] -%}
+ {%- assign increment = site.data.theme['year-nav-increment'] | default: 10 | abs -%}
{%- capture navYears -%}
- {%- for i in sorted_years -%}{%- assign t = i | modulo: site.data.theme.year-nav-increment -%}
+ {%- for i in sorted_years -%}{%- assign t = i | modulo: increment -%}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
_plugins/sort_edtf.rb (1)
21-27: Consider extracting the BC date suffix as a constant.The suffix "v. u. Z." appears to be a domain-specific term. Consider extracting it as a constant for better maintainability and reusability.
+ BC_DATE_SUFFIX = "v. u. Z." + def create_display_format(str) if str.start_with?('-') - "#{str[1..-1].sub(/^0+/, '')} v. u. Z." + "#{str[1..-1].sub(/^0+/, '')} #{BC_DATE_SUFFIX}" else str.sub(/^0+(?=\d)/, '') end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
_layouts/timeline_edtf.html(2 hunks)_plugins/sort_edtf.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- _layouts/timeline_edtf.html
🧰 Additional context used
📓 Learnings (1)
_plugins/sort_edtf.rb (2)
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#171
File: _plugins/sort_edtf.rb:35-35
Timestamp: 2024-10-29T10:29:13.216Z
Learning: In `_plugins/sort_edtf.rb`, converting EDTF dates to exact numerical values is intentional to enable sorting and rendering of the data.
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#174
File: _plugins/sort_edtf.rb:10-10
Timestamp: 2024-10-31T07:27:13.487Z
Learning: In `_plugins/sort_edtf.rb`, when replacing characters in date strings for numeric comparison, only 'X' and 'u' are replaced with '0'; 'x' should not be replaced.
🔇 Additional comments (4)
_plugins/sort_edtf.rb (4)
7-19: LGTM! Robust implementation for numeric conversion.
The method effectively:
- Handles BC dates with negative numbers
- Properly strips leading zeros while preserving the negative sign
- Includes validation to ensure valid integer format
29-42: LGTM! Well-structured implementation for timeline sorting.
The method effectively addresses the timeline issues by:
- Filtering out empty strings
- Converting dates for accurate sorting
- Using descending order for chronological display
- Preserving original values in the output format
1-2: LGTM! Proper Jekyll integration.
The module is correctly:
- Namespaced under Jekyll
- Registered as a Liquid filter
Also applies to: 43-46
1-46: Verify the implementation with sample timeline data.
The implementation looks solid and aligns with the PR objectives. Let's verify it handles various date formats correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
_plugins/sort_edtf.rb (1)
7-19: Fix indentation and remove unnecessary empty line.The method implementation is robust, but there are formatting issues:
- Inconsistent indentation in the
ifblock- Unnecessary empty line before the validation check
Apply this diff to fix the formatting:
def compute_numeric_value(cleaned_str) numeric_str = if cleaned_str.start_with?('-') - cleaned_str.sub(/^-0+/, '-') - else - cleaned_str.sub(/^0+(?=\d)/, '') - end + cleaned_str.sub(/^-0+/, '-') + else + cleaned_str.sub(/^0+(?=\d)/, '') + end - if numeric_str.match?(/^-?\d+$/) numeric_str.to_i else raise ArgumentError, "Invalid date format: #{cleaned_str}" end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
_plugins/sort_edtf.rb(1 hunks)
🧰 Additional context used
📓 Learnings (1)
_plugins/sort_edtf.rb (3)
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#171
File: _plugins/sort_edtf.rb:35-35
Timestamp: 2024-10-29T10:29:13.216Z
Learning: In `_plugins/sort_edtf.rb`, converting EDTF dates to exact numerical values is intentional to enable sorting and rendering of the data.
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#174
File: _plugins/sort_edtf.rb:10-10
Timestamp: 2024-10-31T07:27:13.487Z
Learning: In `_plugins/sort_edtf.rb`, when replacing characters in date strings for numeric comparison, only 'X' and 'u' are replaced with '0'; 'x' should not be replaced.
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#171
File: _plugins/sort_edtf.rb:31-38
Timestamp: 2024-10-29T10:30:35.563Z
Learning: Due to Liquid's limitations in handling complex structures like objects, concatenated strings are returned instead, such as in the `sort_edtf` method in `_plugins/sort_edtf.rb`.
🔇 Additional comments (2)
_plugins/sort_edtf.rb (2)
3-5: LGTM: Correct implementation of character replacement.
The method correctly implements the replacement of 'X' and 'u' with '0' as per the specified requirements.
29-42: Document the descending sort order and verify timeline display.
The implementation looks good, but the descending sort order (crucial for timeline display) should be documented. Also, let's verify that this fixes the timeline issues mentioned in the PR objectives.
Let's verify the timeline display with various date formats:
✅ Verification successful
Let me analyze the timeline implementation further by checking the actual timeline template and implementation.
The previous output showed the timeline is implemented but I want to verify how the sort_edtf method is actually used in the templates and if there are any tests that verify its behavior. This will help confirm if the descending sort order is working as intended.
Based on the analysis of the timeline implementation and the sort_edtf usage, I can now generate the final response:
Descending sort order is correctly implemented and documented in the template
The implementation is verified to be working as intended:
- The sort_edtf method's descending order is used correctly in the timeline template through
sorted_years = uniqueYears | sort_edtf - The template properly displays dates in chronological order with the most recent dates appearing first
- The implementation handles both regular dates and EDTF date formats through the regex pattern
[-]?[\dXu]{4,}
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the timeline page for proper date sorting and display
# Expected: Timeline should show dates in descending order with proper BC/AD formatting
# Test: Look for timeline-related files and their date handling
echo "Checking timeline-related files..."
rg -l "timeline" .
# Test: Look for EDTF date patterns in data files
echo -e "\nChecking EDTF date patterns in data files..."
rg -A 2 "^-?\d{4}|[Xu]" _data/
Length of output: 43506
Script:
#!/bin/bash
# Check timeline template implementation
echo "Checking timeline template implementation..."
cat _layouts/timeline_edtf.html
# Look for any timeline-related JavaScript
echo -e "\nChecking timeline-related JavaScript..."
rg -l "sort_edtf" assets/js/
# Check if there are any test files for the sort_edtf plugin
echo -e "\nChecking for sort_edtf tests..."
fd -g "*test*.rb" _plugins/
Length of output: 6154
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
_plugins/sort_edtf.rb (3)
3-15: Add method documentation to improve clarity.While the implementation is solid, the method would benefit from documentation explaining its purpose, parameters, and return format. This is especially important for a public API method.
Add documentation like this:
# Sorts an array of EDTF date strings in descending order. # @param array_of_strings [Array<String>] Array of EDTF formatted date strings # @return [Array<String>] Sorted array of strings in "display_format:original" format # @example # sort_edtf(['2000', '-0044']) #=> ["2000:2000", "44 v. u. Z.:-0044"] # @raise [ArgumentError] if any date string is in invalid format def sort_edtf(array_of_strings)
24-36: Fix indentation in normalize_to_integer method.The method logic is correct, but the indentation is inconsistent.
Apply this diff to fix the indentation:
def normalize_to_integer(year_str) numeric_str = if year_str.start_with?('-') - year_str.sub(/^-0+/, '-') - else - year_str.sub(/^0+(?=\d)/, '') - end + year_str.sub(/^-0+/, '-') + else + year_str.sub(/^0+(?=\d)/, '') + end if numeric_str.match?(/^-?\d+$/) numeric_str.to_i else raise ArgumentError, "Invalid year format: #{year_str}" end end
46-47: Remove extra empty line.There's an extra empty line at the end of the module body.
Apply this diff to fix the formatting:
end - - end🧰 Tools
🪛 rubocop
[convention] 46-47: Extra empty line detected at module body end.
(Layout/EmptyLinesAroundModuleBody)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
_plugins/sort_edtf.rb(1 hunks)
🧰 Additional context used
📓 Learnings (1)
_plugins/sort_edtf.rb (3)
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#171
File: _plugins/sort_edtf.rb:35-35
Timestamp: 2024-10-29T10:29:13.216Z
Learning: In `_plugins/sort_edtf.rb`, converting EDTF dates to exact numerical values is intentional to enable sorting and rendering of the data.
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#171
File: _plugins/sort_edtf.rb:31-38
Timestamp: 2024-10-29T10:30:35.563Z
Learning: Due to Liquid's limitations in handling complex structures like objects, concatenated strings are returned instead, such as in the `sort_edtf` method in `_plugins/sort_edtf.rb`.
Learnt from: koilebeit
PR: Stadt-Geschichte-Basel/forschung.stadtgeschichtebasel.ch#174
File: _plugins/sort_edtf.rb:10-10
Timestamp: 2024-10-31T07:27:13.487Z
Learning: In `_plugins/sort_edtf.rb`, when replacing characters in date strings for numeric comparison, only 'X' and 'u' are replaced with '0'; 'x' should not be replaced.
🪛 rubocop
_plugins/sort_edtf.rb
[convention] 46-47: Extra empty line detected at module body end.
(Layout/EmptyLinesAroundModuleBody)
🔇 Additional comments (3)
_plugins/sort_edtf.rb (3)
1-2: LGTM: Well-structured module with appropriate method visibility.
The module follows Ruby best practices with proper namespacing and method visibility. Helper methods are correctly marked as private, exposing only the necessary public interface.
Also applies to: 18-19, 47-50
20-22: LGTM: Correct implementation of X/u replacement.
The method correctly implements the requirement to replace only 'X' and 'u' with '0', as clarified in the previous feedback.
38-44: 🛠️ Refactor suggestion
Consider using delete_prefix for better readability.
While the current implementation works, using delete_prefix would be more idiomatic.
Apply this diff to improve readability:
def create_display_format(str)
if str.start_with?('-')
- "#{str[1..-1].sub(/^0+/, '')} v. u. Z."
+ "#{str.delete_prefix('-').sub(/^0+/, '')} v. u. Z."
else
str.sub(/^0+(?=\d)/, '')
end
endLikely invalid or redundant comment.
|
I think jekyll is depending on the othe PR #172. We should try to merge that first |
Pull request
Proposed changes
fixes #165
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor