sillsdev / sillsdev/python-sil-lift

Second companion defining the same range id is dropped, causing false undefined-range-value

Open
#29 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Python
Stars
1
Forks
0
Avg merge
10d 11h
Merged PRs (30d)
6

Description

Summary

When two tracked .lift-ranges companions define the same range id, Lexicon.all_ranges()
keeps the first and drops the second entirely. Every range check runs over that merged
view, so the second companion's elements are never validated — and, because they are also
missing from the merged range, entry values that those elements define are reported as
undefined-range-value.

Reproduction

import sil_lift

lex = sil_lift.Lexicon()

a = sil_lift.RangesFile()
a.add_range("grammatical-info").add_element("Noun")
lex.add_ranges_file(a, href="a.lift-ranges")

b = sil_lift.RangesFile()
b.add_range("grammatical-info").add_element("Verb", parent="Nooun")  # dangling parent
lex.add_ranges_file(b, href="b.lift-ranges")

entry = sil_lift.Entry(id="e1", guid="11111111-2222-4444-8888-111111111111")
entry.lexical_unit["en"] = "e1"
entry.senses.append(sil_lift.Sense(id="s1", grammatical_info=sil_lift.GrammaticalInfo("Verb")))
lex.entries.append(entry)

print([el.id for el in lex.all_ranges()["grammatical-info"].elements])
for p in lex.iter_problems():
    print(p.level, p.code, "|", p.message)
['Noun']
warning undefined-range-value | grammatical-info value 'Verb' is not defined in the range

Two things are wrong there:

  • Verb is defined, by a companion this lexicon tracks. The warning is a false
    positive against the document as a whole.
  • parent="Nooun" matches no sibling id in any normalization, and no range-parent error
    is reported — that element was never looked at.

Cause

Lexicon.all_ranges() merges by exact id, first companion winning:

for ranges_file in self.ranges_files.values():
    for range_ in ranges_file.ranges:
        merged.setdefault(range_.id, range_)

_semantic_problems then iterates all_ranges.values(), so a range id that two
companions define is only ever seen in one of its definitions. Elements are not unioned:
the loser's are dropped, not added.

Still covered

Each companion is separately validated against the ranges schema, and duplicate-guid
scopes per rendered document, so both companions are scanned for guid reuse. It is only
the checks that read the merged view — range-parent, undefined-range-value,
normalization-mismatch — that see one definition.

Options

  1. Report it. A new warning when two tracked companions define the same range id says
    plainly that one definition is being ignored. Cheapest, and honest, but leaves the
    false positive above in place.
  2. Union the elements of same-id ranges in all_ranges(). Fixes both symptoms, but
    changes documented public behavior ("the external definition (from any tracked ranges
    file) is used"), and a synthesized Range would break callers that rely on the merged
    view returning the very object a companion holds — validation does, to address a
    finding to the file that defines it.
  3. Leave all_ranges() alone and widen validation: walk each companion's own ranges
    for the structural checks, and resolve values against the union of ids. Keeps the
    public contract, addresses each finding to the companion it belongs to, and costs one
    more pass.

3 (optionally with 1) looks right, but the tradeoff deserves a second opinion before
anything is written.

Notes

Pre-existing; not introduced by #28, whose ranges_paths identity check is correct as
written (it maps only the range object the merged view actually holds). No corpus fixture
exercises a duplicate range id across companions — real FLEx exports write a single
companion — so this needs a hand-authored folder fixture whichever way it is fixed.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with Lexicon.all_ranges() and _semantic_problems, then reproduce the duplicate range-id case from the issue. Decide between reporting duplicates, unioning elements, or widening validation, and add a hand-authored folder fixture covering the missing range-parent and false undefined-range-value findings; done means the chosen behavior is tested without regressing companion-specific validation.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.