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

Amend field access errors #27

Closed
wants to merge 1 commit into from

Conversation

jakobnissen
Copy link
Member

This PR amends various spec violations, see #24 .

NOTE: These changes ARE breaking, depending on what you call "breaking". Previous code that did not error because of a nonsensical implementations may now error. For example, before this PR, the test BAM file ce1 of BioFmtSpecimens would return true for "hasnextmapped" and would return an out of bounds next mapping position of 0. Now hasnextmapped correctly returns false, and it will raise an error if you attempt to get the next mapping position.

@CiaranOMara
Copy link
Member

Thank you for putting this PR together.

Having thought about this, I believe that there should not be checks in the accessors. Including a check in the accessor doubles the accessor's responsibility and imposes some costs.

For example, there is a performance cost when working with data streams where determinations/decisions about records occur during iteration. In this pattern of usage, checks are performed before the data is accessed and used. Currently, with the check included in the accessor, the check occurs twice.

There is also a flexibility/composability cost; in some sense, a user is gets pushed towards a try/catch paradigm when wanting to avoid double-checks, lest they write their own accessors.

I think the user should be free to choose and compose patterns that suit their needs. So, let us not add checks to the accessors in this fix and work toward removing them for v0.3.

@CiaranOMara CiaranOMara changed the base branch from master to hotfix/field_access August 13, 2020 04:16
@CiaranOMara
Copy link
Member

CiaranOMara commented Aug 13, 2020

I reviewed this PR again today. From my point of view, you've upgraded checks for the presence of values to logical interpretations of present values, which is reasonable if we assume the Record is filled, particularly if an error is thrown when a Record cannot be filled. I think this PR illustrates a clear interpretation and, indeed, expectations of XAM accessors. This sort of upgrade is a development direction in which I would like to head. I interpret the changes in this PR as changes to accessor behaviour, not error corrections.

@jakobnissen, you can switch this PR to the develop branch or close if your work against the develop branch already includes these behavioural changes.

Let me know whether you think I've missed something from the frame that the current has functions are checking for the presence of a value.

@CiaranOMara CiaranOMara deleted the branch BioJulia:hotfix/field_access August 15, 2022 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants