search

bids-standard / bids-specification

Brain Imaging Data Structure (BIDS) Specification
https://bids-specification.readthedocs.io/
Creative Commons Attribution 4.0 International
274 stars 157 forks source link

[ENH] Describe Inheritance Principle in Common Principles #1807

Closed Lestropie closed 4 months ago

Lestropie commented 5 months ago

Currently attempting to revive BEP016, after having had #1003 rejected via #1339 and alternative solution #1280 rejected also.

In #946 I rewrote the Inheritance Principle. Previous text sort of tried to both explain what it was and how it worked and why all together; with that change I focused on defining a set of rules that were both human-readable and actionable for software developers.

However it jumps out at me now that the specification goes straight into those systematized rules with zero introduction of what the Inheritance Principle actually is or why it exists. So here I've tried to give a very succinct explanation. It only needs to be a user-friendly introduction, so doesn't need to be hardcore optimized, happy to take on modifications.

arokem commented 5 months ago

+1 thanks for writing this!

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.92%. Comparing base (01025da) to head (b7a3ab6). Report is 11 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1807 +/- ## ======================================= Coverage 87.92% 87.92% ======================================= Files 16 16 Lines 1375 1375 ======================================= Hits 1209 1209 Misses 166 166 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Lestropie commented 5 months ago

Good catch @yarikoptic.

Re-running CI has overcome the prior failure due to a dead link.

effigies commented 4 months ago

@Lestropie Are you happy with the changes? I think both the current text and the proposed fix have been up long enough that there's been plenty opportunity to object.

Lestropie commented 4 months ago

Suggest a minor extension to contextualise why such an order is important, partly because it gives the opportunity to recommend against it. Happy for you to merge with or without b7a3ab625a7c1994d3984f3eb79c4811449e7e2b.

effigies commented 4 months ago

I would not add a recommendation here. That should be paired with a validator warning, and I'm not positive people would agree that it should be a warning. Nor am I sure how much work it would be to validate.

effigies commented 4 months ago

@Lestropie I did drop b7a3ab6 because that had not gotten general review and I think it was a non-trivial change. Feel free to propose it for separate review.