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

[INFRA] Customize code suggestions & completions by adding a functionSignatures file #42

Merged
merged 7 commits into from
Jan 8, 2021

Conversation

mslw
Copy link
Contributor

@mslw mslw commented Sep 10, 2020

I would like to propose a minor Quality of Life improvement (at least for Matlab users - I don't think Octave uses the same mechanism) by providing hints for tab-autocompletion. This can be done by adding a functionSignatures file (docs), which affects the behavior of Matlab's Command Window, Editor and Live Editor.

The file included in the pull request causes Matlab to:

  • autocomplete paths in calls to bids.layout, bids.query and bids.validate
  • suggest a choice of 'data', 'metadata', 'sessions' etc. as a second argument to bids.query (typing bids.query(BIDS, <tab>) shows the suggestions)

I am not sure if you will consider having autocompletion hints a useful feature, so the provided annotations are rather minimal (i.e just a few arguments, and none for bids.report), although that is already quite handy for my own use.

Possible additions could include eg. adding all possible keys for key-value pairs in bids.query (although - is there a limited set?) and annotating bids.report. I would be happy to do more work if you think an expansion would be needed.

The file is used to provide Matlab with information required for
code suggestions and completions. Only the required arguments for
layout, query and validate were annotated.
@gllmflndn
Copy link
Collaborator

Thank you @mslw, it looks great. It is not something I have been using so far but that's a welcome addition. You are right this is a MATLAB-specific thing and is unlikely to work with Octave any time soon. The only "issue" is to make sure that the signatures remain in sync with the definition of the functions if and when they evolve.
Do you want to merge this as is and we can work on adding more features later or are you keen to do this work for this PR, e.g. for bids.query?

@Remi-Gau
Copy link
Collaborator

Yes this looks interesting. As long as the Octave functionality is not impaired I am in favor of any "Plus" for matlab.

In terms of workflowto keep things up to date: using a pull-request template with a reminder to update the function signature could be an "easy" and gentle reminder.

More involved would be to have something on the CI side that flags functions with no definitions.

We are going to need some documentation I think...

@mslw
Copy link
Contributor Author

mslw commented Sep 30, 2020

Thanks for the kind words and sorry for the long silence - I went on holiday soon after submitting the PR.

I didn't initially do bids.report because I misunderstood something about the arguments. But now I see that subj & sess are numbers of subject/session (or vectors thereof), rather than their names (am I right?), so I think I should first add an annotation for this function.

Regarding further changes - for me having the path autocomplete was the biggest convenience (and the reason why I added the json to my local copy of bids-matlab...). I think having all the entities for the bids.query has smaller benefits and may be harder to keep up to date, so I am not sure what is the best decision.

One additional thing I was thinking about was adding a test, but the best I can come up is merely checking whether the functionSignatures file is syntactically correct json (and maybe whether the top level contains the required functions?). Seeing that the test run in Octave - that would probably have to call bids.util.jsondecode on the file and see if it crashes or not? What do you think?

So to answer the question about merging - I will try to annotate bids.report this week and I would be fine with merging at that point already; for the other elements I would rely on your judgement.

Subj, Sess & Run are described as numeric. It appears that "Subj"
and "Run" could be ["numeric", "scalar"], meaning (numeric & scalar),
but it only affects what variables would be suggested by tab completion
so perhaps less is more here.
In the 2nd parameter to bids.query, listing char & string together
did not do what I thought. From the docs:
"For the inner lists, MATLAB uses a logical AND of the values.
For the outer list, MATLAB uses a logical OR of the values."
@Remi-Gau Remi-Gau added the infrastructure Anything to do with automation, continuous integration... label Oct 29, 2020
@Remi-Gau Remi-Gau changed the title Customize code suggestions & completions by adding a functionSignatures file [INFRA] Customize code suggestions & completions by adding a functionSignatures file Oct 29, 2020
@Remi-Gau Remi-Gau added this to Review in progress in General Kanban Oct 29, 2020
@Remi-Gau Remi-Gau self-assigned this Nov 11, 2020
@Remi-Gau Remi-Gau self-requested a review November 11, 2020 06:40
@Remi-Gau
Copy link
Collaborator

Finally taking some time to have a look at this.

@mslw
I have sent you a PR with a suggestion of reformatting of the JSON for pure legibility issues.

I am starting to play around with this so bear with me: but I suspect that other people unfamiliar with this feature might benefit from some pointers.

Starting with this in the command line, Tab autocompletes works fine.

bids.layout(fullfile(

But I am unclear on how to make this work with the other functions.

Can you give an example?

@Remi-Gau
Copy link
Collaborator

I didn't initially do bids.report because I misunderstood something about the arguments. But now I see that subj & sess are numbers of subject/session (or vectors thereof), rather than their names (am I right?), so I think I should first add an annotation for this function.

Maybe don't over-engineer things on bids.report just yet. I expect this part of the code to change quite a bit still, so don't put work into that would have to be undone later. Having a place for the function signature will already be a start.

Regarding further changes - for me having the path autocomplete was the biggest convenience (and the reason why I added the json to my local copy of bids-matlab...). I think having all the entities for the bids.query has smaller benefits and may be harder to keep up to date, so I am not sure what is the best decision.

Well I think that something like this should be part of our contributing guideline and realease protocols to make sure that people know about them when they submit code and for us to make sure we check those function signatures are still valid before realease.

One additional thing I was thinking about was adding a test, but the best I can come up is merely checking whether the functionSignatures file is syntactically correct json.

I would say that making sure we have a valid JSON is already good. 😉 If we had tons of them we would use some other validator but that will do for now.

(and maybe whether the top level contains the required functions?).

Oh that would be nice indeed!

Seeing that the test run in Octave - that would probably have to call bids.util.jsondecode on the file and see if it crashes or not? What do you think?

Yes that is a good point. But it seems that you already read the JSON with bids.util.jsondecode, no?

automatic reformating of JSON with prettier
@mslw
Copy link
Contributor Author

mslw commented Nov 18, 2020

Regarding the usage examples

Starting with this in the command line, Tab autocompletes works fine.
bids.layout(fullfile(
But I am unclear on how to make this work with the other functions.

Can you give an example?

Please bear in mind that I work in matlab GUI, so by "autocompletion" I mean both pop-ups with suggestions and actual filling-in of things I started typing, as shown below:

Zrzut ekranu 2020-11-18 o 13 53 59

With bids.layout (argument with type: folder) the actual deal is that even without fullfile (for which matlab already knows it should expect a path), Tab-autocompletion of a path will work if bids.layout is annotated, i.e. :

bids.layout('/Volumes/<TAB> will be tab-completing with directories I have on my drive(s)

That being said, real life benefit isn't that big - I may benefit from my path being autocompleted when I'm doing some interactive work, but in most of my scripts I would build a variable storing the path before I get to calling something like bids.layout(BIDS_ROOT) anyway.

For bids.query, two things happen. For the first argument, workspace variables of type struct will be suggested (marginally useful). For the second argument, a popup will show all the available options (specified as "choices" in JSON), i. e. "data", "metadata", etc (much more useful). Also, if I start typing one of those, it would tab-autocomplete. That is:

bids.query(<TAB> - a popup with struct variables already defined in the workspace appears
bids.query(b, <TAB> - opens a double quote and shows a list of options (as in the picture)
bids.query(b, 'me<TAB - autocompletes to bids.query(b, 'metadata'

As a side note, same thing might also happen for the subsequent key-value arguments if they were also listed in the JSON, but I think that would be too much - for me having the "mode switches" (2nd argument) was the most useful thing.

For others, bids.validate is same as layout; bids.report will also try to autocomplete path in the 1st argument if you type a string, and for subsequent argument will try suggesting variables which are numeric if you hit tab before typing anything. For me these behaviours are not very useful, and the two functions are annotated mostly for completeness.

I guess part of the question is whether these need to be documented - perhaps for the developers, as you suggest, but from the user perspective the presence of annotation mostly mimics the behaviour of built-in functions.

@mslw
Copy link
Contributor Author

mslw commented Nov 18, 2020

Thank you for taking time to look closer at the proposed additions. Here are my comments to the remaining points you raised.

Json formatting
I accepted the changes (f1501fe). Personally I am used to lesser indents, but the standards of Prettier are perfectly fine, too.

bids.report

Maybe don't over-engineer things on bids.report just yet. I expect this part of the code to change quite a bit still, so don't put work into that would have to be undone later. Having a place for the function signature will already be a start.

Agreed. I managed to come up with some minimal annotations and I think I am happy with where we're at right now. As mentioned earlier, I see less utility in annotations for report compared to layout & query. If anything, they should provide slight assistance when calling the functions in matlab, not cause headaches for development.

Keeping annotations up to date

Well I think that something like this should be part of our contributing guideline and realease protocols to make sure that people know about them when they submit code and for us to make sure we check those function signatures are still valid before realease.

I can only agree - and perhaps just saying "when adding top-level functions or modifying their inputs, make sure that they have valid entries in functionSignatures.json (insert link to mathworks docs)" would be enough. I guess it is just conceptualization for now, or should I be adding it as part of this PR?

Tests
Yes, I used bids.util.jsondecode to check if the file can be loaded (btw, native json support seems to be in line for a future release of Octave). As for checking the field names - all I came up with was listing them manually like so:

% Check if all top-level functions are annotated
fields = {'bids_layout', 'bids_query', 'bids_validate', 'bids_report'};
assert(all(isfield(signatures, fields)));

but if the aim was to safeguard against new unannotated functions, it won't help, as they would have to be manually included in the test. So I am not sure if it would help or not (and didn't commit).

@Remi-Gau
Copy link
Collaborator

For bids.query, two things happen. For the first argument, workspace variables of type struct will be suggested (marginally useful). For the second argument, a popup will show all the available options (specified as "choices" in JSON), i. e. "data", "metadata", etc (much more useful). Also, if I start typing one of those, it would tab-autocomplete. That is:

bids.query(<TAB> - a popup with struct variables already defined in the workspace appears
bids.query(b, <TAB> - opens a double quote and shows a list of options (as in the picture)
bids.query(b, 'me<TAB - autocompletes to bids.query(b, 'metadata'

hey @mslw

Thanks for that explanation!! This is SO much clearer than the one on MathWorks !! 😉

I think the key element I was missing was this: "popup with struct variables already defined in the workspace appears".

Will test on my computer as soon it is done crunching numbers(*): but this is looking good to me. I will open side issues to keep track of updating the "contributing.md" to mention the signatures. No need to add this to this PR.

(*) An unfortunate keystroke had me run a rm instead of a ls from my command history and now I have to re-run tons of stuff... Sigh.

@Remi-Gau
Copy link
Collaborator

A month later I finally have the time to check this out... So sorry. 😢

OK I am not sure why but most of my tab completion just throw an error:

Unknown kind value "ordered"

This was solved by removing any mention of "kind": "ordered" in the function signature...

Could this be a Matlab version issue? Testing on 2017a at the moment.

@mslw
Copy link
Contributor Author

mslw commented Jan 5, 2021

Huh, the error sounds strange. Thanks for taking a closer look - this is becoming more complex than I expected.

I'm on 2019b right now but I will check on 2017a in the coming days. Theorycrafting below.

It might indeed be related to Matlab version. From what I could find on the web, it seems that the mechanism with functionSignatures.json was introduced somewhere around 2016b (initially undocumented). Perhaps the specs for the json file evolved between 2016 and now? The current docs say that schema version can be specified (and 1.0.0 is assumed by default), which implies that other versions may have existed.

As a sidenote, this means versions prior to 2016 would simply not make use of the file.

In case you beat me to it, there are two quick things I would try in R2017a:

  • see if setting schema version at the beginning produces a different error:
{
    "_schemaVersion": "1.0.0",
    "bids.layout": {
         ...
  • see if changing "kind": "ordered" to "kind": "positional" helps: the difference between the two is very subtle, and while current docs mention both with emphasis on the former, this 2017 blog post mentions only the latter.

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Jan 7, 2021

Huh, the error sounds strange. Thanks for taking a closer look - this is becoming more complex than I expected.

Sorry about that. :-(

I'm on 2019b right now but I will check on 2017a in the coming days. Theorycrafting below.

It might indeed be related to Matlab version. From what I could find on the web, it seems that the mechanism with functionSignatures.json was introduced somewhere around 2016b (initially undocumented). Perhaps the specs for the json file evolved between 2016 and now? The current docs say that schema version can be specified (and 1.0.0 is assumed by default), which implies that other versions may have existed.

As a sidenote, this means versions prior to 2016 would simply not make use of the file.

I think we are OK with that. as long as stuff don't break compatibility of the core functionality with older Matlab or Octave I think it can be fine to have things like this. :-)

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Jan 7, 2021

In case you beat me to it, there are two quick things I would try in R2017a:

* see if setting schema version at the  beginning produces a different error:
{
    "_schemaVersion": "1.0.0",
    "bids.layout": {
         ...

That did not change anything.

* see if changing `"kind": "ordered"` to `"kind": "positional"` helps: the difference between the two is very subtle, and while [current docs](https://www.mathworks.com/help/matlab/matlab_prog/customize-code-suggestions-and-completions.html) mention both with emphasis on the former, [this 2017 blog post](https://undocumentedmatlab.com/matlab/wp-content/cache/all/articles/user-defined-tab-completions-take-2/index.html) mentions only the latter.

That seems to "work". Changed all the "kind": "ordered" of the signature of report to positional and now I don't get any error on tab completion.

@mslw
Copy link
Contributor Author

mslw commented Jan 8, 2021

Huh, the error sounds strange. Thanks for taking a closer look - this is becoming more complex than I expected.

Sorry about that. :-(

If anything, it's on Matlab. I think it's for the better that you found issues at this stage.

That seems to "work". Changed all the "kind": "ordered" of the signature of report to positional and now I don't get any error on tab completion.

So it would seem that ordered wasn't added until mid-2017 at least. Interesting... I think positional is perfectly fine in the context of bids.report given the definitions are:

Ordered - Argument is optional, and its location is relative to the required and preceding optional arguments in the signature object.
Positional – Argument is optional if it occurs at the end of the argument list, but becomes required to specify a subsequent positional argument. Any positional arguments must appear with the required and ordered arguments, before any namevalue arguments.

I am changing to positional to avoid causing trouble in Matlab releases from 2016/17.

Change argument kind from ordered to positional in the signature for report.
Prevents errors on Matlab R2017a (probably 2016 as well, kind=ordered likely introduced later).
@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Jan 8, 2021

Awesome thanks!

This is good to merge for me.

Will open a side issue to make sure to remember to add "keeping the function signatures up to date" part of our "release protocol".

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Jan 8, 2021

As this is more "infrastructure" than purely code related I will merge now rather than having the "approval" period of our decision making process describes (also I think that this has been long on the back burner for way too long!!!) 😉

@Remi-Gau Remi-Gau merged commit 36c2233 into bids-standard:master Jan 8, 2021
General Kanban automation moved this from Review in progress to Done Jan 8, 2021
@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Jan 8, 2021

@all-contributors please add @mslw for infra, ideas, code

@allcontributors
Copy link
Contributor

@Remi-Gau

I've put up a pull request to add @mslw! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Anything to do with automation, continuous integration...
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants