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

Move +bids/private stuff to +bids/+internal? #25

Merged
merged 2 commits into from
Oct 5, 2020
Merged

Move +bids/private stuff to +bids/+internal? #25

merged 2 commits into from
Oct 5, 2020

Conversation

apjanke
Copy link
Collaborator

@apjanke apjanke commented Dec 20, 2019

What do you think of moving the package-private functions in +bids/private to a new +bids/+internal package?

This would allow these functions to be directly tested and played with during development, but would still indicate to users of the library that they're for BIDS-MATLAB's internal use and shouldn't be called from client code. This "internal" package name is a pretty well-established convention these days. I've seen it in both Java and Matlab code, including MathWorks' own code.

This also removes the need to do the extra addpath step under Octave to work around its handling of private directories inside packages. This PR updates the .travis.yml file to reflect that.

@apjanke apjanke changed the title Move +bids/private stuff to +bids/+internal Move +bids/private stuff to +bids/+internal? Dec 21, 2019
@robertoostenveld
Copy link
Collaborator

it is indeed used a lot in MATLAB toolboxes. I am fine with this.

@apjanke apjanke mentioned this pull request Jan 1, 2020
@Remi-Gau
Copy link
Collaborator

I will defer to people with more experience on this (AKA @robertoostenveld) so I would be in favor.

@gllmflndn
Copy link
Collaborator

The issue in Octave with private directories inside +packages has now been fixed and will be available in the next release.

Changing private to +internal is a switch from a hard constraint to a soft one on who can call the function. I initially preferred private as the rule is then explicit and enforced but it is true that +internal will allow to write more fine-grained unit tests and will allow these functions to be called throughout the +bids package (e.g. from +bids/fcn1.m and +bids/+util/fcn2.m so that's a 👍 for me too.

Allows these functions to be tested directly, and removes the
need for the Octave path hack.
@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Oct 3, 2020

Will merge this one as well in 24 hours.

Speak now or forever... send a PR later.

@Remi-Gau Remi-Gau merged commit c847e28 into bids-standard:master Oct 5, 2020
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.

4 participants