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

fix default for BiblicalTermsListSetting #125

Merged
merged 1 commit into from
Oct 8, 2024
Merged

Conversation

mshannon-sil
Copy link
Collaborator

@mshannon-sil mshannon-sil commented Oct 7, 2024

The reason that BiblicalTermsListSetting wasn't allowed to be optional was because the default was set to "", but the check to see if a BiblicalTermsListSetting exists in the Settings.xml uses if biblical_terms_list_setting is not None. I've removed the explicit default so that if it's not found, it assigns None, and the subsequent check will work properly. This bug is only in machine.py, not in Machine.

I tested this by temporarily removing the BiblicalTermsListSetting line in the Settings.xml of the Tes folder, and then verifying that the change in defaults successfully changed tests from failing to passing. However, there were a couple tests that depended on the contents of BiblicalTermsListSetting not being empty/default, so I had to undo that change in Settings.xml when committing.

Should I create test case(s) for the parse() method in ParatextProjectSettingsParserBase along with this bug fix? I'd imagine that would require another test file since there isn't one for ParatextProjectSettingsParserBase, and then adding a corresponding test file in Machine as well to keep the repos matching.


This change is Reviewable

@mshannon-sil mshannon-sil added the bug Something isn't working label Oct 7, 2024
@mshannon-sil mshannon-sil requested a review from ddaspit October 7, 2024 20:51
@mshannon-sil mshannon-sil self-assigned this Oct 7, 2024
@mshannon-sil mshannon-sil linked an issue Oct 7, 2024 that may be closed by this pull request
Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mshannon-sil)

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.14%. Comparing base (7a2123f) to head (e2b3206).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #125   +/-   ##
=======================================
  Coverage   88.14%   88.14%           
=======================================
  Files         273      273           
  Lines       16010    16010           
=======================================
  Hits        14112    14112           
  Misses       1898     1898           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mshannon-sil mshannon-sil merged commit e0e3f08 into main Oct 8, 2024
14 checks passed
@mshannon-sil mshannon-sil deleted the #124_biblical_terms branch October 8, 2024 17:56
@mshannon-sil mshannon-sil linked an issue Oct 8, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

Optional BiblicalTermsListSetting element in the Settings.xml file Optional BiblicalTermsListSetting element
3 participants