Pull erikhuizinga/legtools/master into StackOverflowMATLABchat/legtools/master

[erikhuizinga]

Summary

This is a large pull request fixing some bugs and adding a few new features, while remaining fully compatible with the previous release.

Bugs fixed

1. remidx in remove method exceeds number of legend entries

Example

figure
ezplot sin
legend sin
legtools.remove(legend,2)
% 2 exceeds the number of legend entries, which is 1

This commit fixes the bug.

2. handlecheck method improperly handles an empty legend object

handlecheck improperly throws an error when an empty legend object is passed to it. This
commit implements detection of an empty legend.

3. objtodelete is never set in remove method

Example

figure
fplot(@cos)
hold on
fplot(@sin)
legend cos sinlegtools.remove(legend,2)

Results in error:

Undefined function or variable 'objtodelete'.
Error in legtools.remove (line 134)
                delete(objtodelete);

This commit fixes the bug.

4. append method improperly rearranges Legend property PlotChildren

The append method contains a bug when it appends to a legend that has been permuted previously or if the parent axes children have been sorted differently after the legend has been modified. append assumes the parent axes children are in the same order as the current legend PlotChildren, which may not be the case. For example, after permuting the legend entries, this order may have changed.

Example

figure
fplot(@cos)
hold on
fplot(@sin)
legend cos sin
legtools.adddummy(legend,'dummy','ok')
legtools.permute(legend,[3 2 1])
legtools.remove(legend,2)
legtools.append(legend,'sin')

Which results in (notice the incorrect colouring of the legend entries):

Note that the 3rd bug happens before this bug occurs, so you won't see this result on release v2.2. If you branch off 354a35e you can see the bug in action.

This commit fixes the bug.

Improvements

• Optimise code
  • Reduce code redundancy
  • Annotate with better comments
  • Improve readability
• Optimise help
  • Update help for new functionality
  • Format help to be easier to understand
  • Incorporate less technical explanations, hence easier for non-advanced MATLAB users
  • Format help to be more in line with help for MATLAB's built-in functions
  • Update Readme.md with latest help
  • Add new badge to Readme.md
  • Add new images to Readme.md
• Remove warning from remove method that was given when deleting the legend, because this is the expected behaviour (as explained in the help) and thus should not throw a warning
• Improve error text format
  • Make full sentences with capital letters and punctuation while being imperative like in most MATLAB functions
• Implement more input argument checks
• Improve detection of dummy legend entries
• Respect current hold state of parent axes of legend

New functionality

• Support string data type introduced in MATLAB R2016b
• Enhance plotParams input argument of adddummy method
  • Support multiple formats
  • Make plotParams optional

[sco1]

General remarks

• Pull requests should be focused on one specific topic. Pull requests are difficult to review and making changes to multiple areas in the same request makes it difficult to follow logically and difficult to merge appropriately.
• Commits whose purpose cannot be sufficiently explained in the title of the commit should include details in the body of the commit. This allows others to understand what the change is and why it was made, both because they may not be familiar with the circumstances or they're looking back through the history to understand how development proceeded. Statements like "optimize" and "improve" should include enough supporting information so someone can look at it and understand what the optimization or improvement is. This should be done consistently.
• It's much easier for folks to follow the development history if commits are significant. A commit solely removing a couple brackets or spaces either shouldn't be a thing or it should be squashed prior to the PR.
• Because a git repository contains the complete history, files committed to the repository are there permanently. While some compression and differencing techniques can reduce the size of text files, git does not natively handle binary files (images, exe's, etc.) very well so they can quickly bloat the repository size, even if they are later deleted.
• GitHub's issue tracker is very useful! It allows folks to bring issues to the attention of the developer for comments & fixing. Sometimes the developer might have fixes in the works!

Marginally More Detailed Remarks

MATLAB's default indentation on line continuation is unclear and the choice of aligning openings and closures for statements spanning multiple is intentional. This is consistent with other languages.

No:

error('legtools:permute:TooManyIndices', ...
    'Number of values in order must match the number of legend strings' ...
    );

Yes:

error('legtools:permute:TooManyIndices', ...
      'Number of values in order must match the number of legend strings' ...
      );

Appending () to method calls is intentional and shows the user, at a glance, that the command is intending to call a method or function rather than trying to address a property or field. This is consistent with other languages.

Prepending inline comments with 2 spaces rather than 1 is an intentional deference to PEP 8.

The use of error rather than assert is intentional. While there are differing opinions for other languages, the prevailing opinion is that, in general, assertions should not make it to production code.^1,2 For MATLAB's purposes they're essentially identical. Other than the potential for difference in code quality for C code generated from MATLAB³, there isn't much evidence supporting a significant performance advantage for either method. This brings it do a largely stylistic choice, and the preference here is for error.

Bringing implicit input support to adddummy does not align with the intent of the method. The purpose of adddummy is to provide legend entries for objects not supported by legend, so a linespec cannot be automatically generated from the object. In order for the legend entry to match the object, the user must specify a linespec. By removing the explicit requirement we request that MATLAB arbitrarily decide on a format for the legend entry. This is not desired and will not be implemented.

Path forward

There are many good bug fixes here, both ones that have been on my plate to take care of and also ones I hadn't anticipated. Unfortunately, with the scope of this PR being so broad they are intermingled with changes I have no intention of making that I doubt it will be possible to merge without significant cleanup. I will be splitting the major bugs off into issues and incorporating fixes to them individually, with appropriate attribution.

[erikhuizinga]

Thanks for your elaborate reply. This helps me learn to use Git and GitHub, as well as look beyond some of the quirks MATLAB uses/has. Sorry for the time it must have taken you to go over my large PR. Luckily for me, you took the time yourself to provide me with feedback. Next time I'll open issues.

By the way: when is it better to leave in-line comments, when to create an issue? An issue can mention a specific point in some code, but a comment in the code on the specific location can pinpoint a problem.

And are issues to be opened about anything, for example the minor stuff many of my commits ‘fix’?

[erikhuizinga]

And in this case, would it have been possible for me to fix each bug separately? Like so:

I create issues for the bugs I intend to fix, we communicate about the issues, we decide I am fixing them. Then, could the following happen (assuming one bug's code isn't dependent of another)?

Example

A is the point your repo is currently at
I fork it into my own:

A
 \
  A*

I then fix one bug and squash all necessary commits into one:

A
 \
  A*--B

Then I create a PR just for that bug, referencing the corresponding issue. If it's accepted, were at:

A-------B
 \     /
  A*--B*

I then fix another bug, but this could happen in two moments of time: either when B* has been pulled into your repo, or before B* has been created. In the latter case, could this have worked (assuming C* can be independent of B*)?

A-------B------C
 \     /      /
  A*--B*     / 
   \        /
    C*------

[sco1]

No worries, it's great to get additional eyes on my code! While I do catch many bugs, I'm also looking at it as the primary author so it's easy to make assumptions that aren't necessarily correct and lead to issues elsewhere.

By the way: when is it better to leave in-line comments, when to create an issue? An issue can mention a specific point in some code, but a comment in the code on the specific location can pinpoint a problem.

Issues are preferred, as inline comments unnecessarily clutter the repository's history. GitHub allows you to link directly to a commit, and even directly to a line number. For example, this link goes directly to line 53 of legtools.m in the project's first commit. For more info, see Mastering Markdown and How to link to specific line number on github.

And are issues to be opened about anything, for example the minor stuff many of my commits ‘fix’?

Yep! Issues aren't just for reporting bugs, they're great ways to communicate with the developers. Some projects may have different contribution guidlines, but right now this project is completely open.

[sco1]

And in this case, would it have been possible for me to fix each bug separately?

For the most part, yes. The issue of diverging branches is an important one, and it's mainly accounted for during review/merging of the PR. If the PRs are structured so that you're addressing specific portions of the code, merge conflicts should be minimal and the review process should be straightforward.