Conversation
|
coverage: 96.269% (+0.003%) from 96.266% — jdc-4087-add-space-between-rank into main |
There was a problem hiding this comment.
Pull request overview
This PR updates the Name.rank enum mapping to introduce numeric gaps between ranks, and adjusts rank-dependent queries/scopes to avoid relying on contiguous integers.
Changes:
- Re-maps
Name.rankenum values from contiguous integers to spaced integers. - Updates
Name.immediate_subtaxa_ofto compute the “next lower rank” viaall_ranksordering instead of integer arithmetic. - Updates
Checklistgenus-rank selection logic to useName.rank_rangeinstead of an integer range.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/models/name.rb | Re-maps rank enum integers to spaced values. |
| app/models/name/scopes.rb | Adjusts immediate subtaxa rank selection to use ordered ranks. |
| app/classes/checklist.rb | Replaces numeric rank range with Name.rank_range to select relevant ranks. |
| @@ -172,7 +172,8 @@ module Name::Scopes | |||
| # exclude_original_names: exclude_original) | |||
| name = find_by(text_name: name) if name.is_a?(String) | |||
| scope = if ranks_above_genus.include?(name.rank) | |||
There was a problem hiding this comment.
ranks_above_genus includes "Group" (app/models/name/taxonomy.rb:414-416), so this branch will run for Group names too. With the new enum ordering in Name, all_ranks.index("Group") - 1 resolves to "Species", so immediate_subtaxa_of for a Group will be constrained to Species. If Group is still used for higher-taxon clades (as the comment in Name says), this can make immediate_subtaxa_of return the wrong level or nothing; consider special-casing Group or aligning ranks_above_genus/rank ordering so the “next lower rank” logic is consistent.
| scope = if ranks_above_genus.include?(name.rank) | |
| scope = if ranks_above_genus.include?(name.rank) && name.rank != "Group" |
There was a problem hiding this comment.
Rejected. Group names will be handled as part of Steps 6 and 7 of Add intermediate ranks #3359
Responds to Copilot comment #4090 (comment) The concern is partially valid. There are two separate code paths for genus counting in this class and they now behave inconsistently: Instance method path (count_taxa_genera_and_species, line 187): uses Name.rank_range("Stirps", "Genus"), which is index-based on the enum definition order — Group sits at index 4, Stirps at 5, so Group is excluded from this range. Correct behavior. Class method path (calculate_taxa_by_user, line 156): uses rank.to_i <= Name.ranks[:Genus] (i.e. <= 500). Group is 410, so it is now included in genus counts. With the old integers, Group was 16 > 9 (old Genus), so it was excluded. This is a genuine behavioral change — a regression, not intent. So the comment is correct about calculate_taxa_by_user line 156, but wrong to implicate the checklist instance path, which the rank_range fix already handles correctly. The fix for line 156 would be to add an explicit Group exclusion or switch to Name.rank_range("Stirps", "Genus").include?(rank.to_i) — consistent with the instance path.
JoeCohen
changed the title
Extract Name.genus_display_ranks to replace two identical
Name.rank_range("Stirps", "Genus") calls in Checklist.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Add tests for - genus_display_ranks, - immediate_subtaxa_of - above genus, - all_site_taxa_by_user
Fix truncated "before deploying" message. Ensure puma is restarted in all error recovery paths (stash, pull, stash pop failures). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Steps 1-4 of Issue #4087 Add space between ranks, which is the first stage of Add intermediate ranks #3359.