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

[ENH] Describe Inheritance Principle in Common Principles #1807

Merged

Conversation

Lestropie
Copy link
Collaborator

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.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

This reads very cleanly to me.

@arokem
Copy link
Collaborator

arokem commented Apr 26, 2024

+1 thanks for writing this!

Copy link
Collaborator

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Overall great, just one minor suggestion

src/common-principles.md Outdated Show resolved Hide resolved
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

@Lestropie
Copy link
Collaborator Author

Good catch @yarikoptic.

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

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

This is a great addition, thanks a lot!

src/common-principles.md Outdated Show resolved Hide resolved
@effigies
Copy link
Collaborator

@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
Copy link
Collaborator Author

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 b7a3ab6.

@effigies
Copy link
Collaborator

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 effigies force-pushed the describe_inheritance_principle branch from b7a3ab6 to 8154a6b Compare May 22, 2024 13:41
@effigies effigies changed the title Common Principles: Describe Inheritance Principle [ENH] Describe Inheritance Principle in Common Principles May 22, 2024
@effigies effigies merged commit 606c3fd into bids-standard:master May 22, 2024
42 of 45 checks passed
@effigies
Copy link
Collaborator

@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.

@Lestropie Lestropie deleted the describe_inheritance_principle branch May 24, 2024 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants