octave-patch-tracker
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Octave-patch-tracker] [patch #9953] [octave forge] (mapping) angl2str


From: Philip Nienhuis
Subject: [Octave-patch-tracker] [patch #9953] [octave forge] (mapping) angl2str
Date: Sat, 29 Aug 2020 09:37:57 -0400 (EDT)
User-agent: Mozilla/5.0 (Windows NT 6.1; rv:52.0) Gecko/20100101 Firefox/52.0

Update of patch #9953 (project octave):

                  Status:                    None => Need Info              
             Assigned to:                    None => philipnienhuis         

    _______________________________________________________

Follow-up Comment #8:

@ricardofantin:
I've just put your round.m as subfunction "round_a" into your angle2str.m +
replaced the calls and all 490 tests pass.

That doesn't inhibit your procedure to improve round.m in core octave, it's
just meant to get a working angl2str.m into mapping without the issues
outlined in comment #7.
(indeed Octave's round.m can be improved. I'll support a separate patch for
that but I'm not the one to decide about it :-) )

If you agree with this "fix" for round we can incorporate your angl2str.m into
the mapping package that way and replace the one written by me (no problems
:-) yours is much more Matlab-compatible). In that new subfunction "round_a"
all input validation can be dropped as well.

If the rounding you need can be done in an alternative way (I think that's
possible) it can be tackled later on, maybe even in some much later mapping
package release. As pkg maintainer I value correct output, Matlab
compatibility and complete package functionality (in that order) somewhat
higher than speed and "superior" coding (which is always a subjective
interpretation). IOW "first get it in then it can be improved later on".

That said, I spotted a few style & logic issues but I can tackle those after
first committing your angl2str.m. I mention them here just FYI:
* print_usage after an error statement. print_usage() never gets called then
as execution breaks at the preceding error statement. In Octave print_usage is
rarely if ever used anyway when checking input arg contents and your suggested
error messages are sufficiently informative, if somewhat verbose.
* tolower => lower
* missing line continuation markers esp. in compound if statements. Sometimes
this can give rise to hard to uncover bugs
* input validation should first check type, e.g., just try 'angl2str (5, 5)'
or 'angl2str (5, "ew", 5)'. 
* double rather than single quotes here and there
* too long lines
* switch statements should include an 'otherwise' clause
* precede comment lines with '##' rather than single '#'
* + a few typos.


So, what do you think?

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/patch/?9953>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/




reply via email to

[Prev in Thread] Current Thread [Next in Thread]