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

Use and document default numerical formatting #191

Merged
merged 6 commits into from
Feb 28, 2019
Merged

Use and document default numerical formatting #191

merged 6 commits into from
Feb 28, 2019

Conversation

cbm755
Copy link
Collaborator

@cbm755 cbm755 commented Feb 26, 2019

No description provided.

Run `format()` before starting tests.  This ensures docs match the
out-of-the-box formatting that users see.  By actively calling `format`,
we ensure consistency for developers who may have custom formatting in
their config files.  A downside is that we overwrite user's custom
formatting.

Partially fixes #183.
@cbm755
Copy link
Collaborator Author

cbm755 commented Feb 26, 2019

@mtmiller and @catch22 how does this look for #117 and #183?

@catch22 I haven't tested on Matlab yet.

Something that worries me is I don't understand why save_format and save_spacing aren't cleared by tests that do clear all...

@cbm755 cbm755 added this to the 0.7.0 milestone Feb 26, 2019
@catch22
Copy link
Collaborator

catch22 commented Feb 27, 2019

Thank you! Perhaps @mtmiller can advise on the clear all question?

Unfortunately I don't have MATLAB available currently, otherwise I would be happy to run the tests.

@mtmiller
Copy link
Collaborator

why save_format and save_spacing aren't cleared by tests that do clear all

That is only because of function scope, clear can only clear variables in the current scope. A clear all in a test block runs in the scope of doctest_run_tests, the local variables in doctest are safe.

inst/doctest.m Outdated
@@ -285,12 +310,35 @@
summary.num_tests = 0;
summary.num_tests_passed = 0;

% stash user's formatting
if (is_octave)
if (compare_versions(OCTAVE_VERSION(), '4.3.0', '>='))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Recommend changing this to 4.4.0, 4.3.0 was a development version, shouldn't exist anywhere now

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it still be more logical to test for 4.3.0 since the "old" way of doing it will fail there (if I understand gnu-octave/symbolic#713 (comment) right)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIRC, there are probably some range of commits where neither works...

People running old dev versions get to keep both pieces :-)

Or we could bump our min version to 4.4 and simplify this...?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a try-catch version with no version testing: personally I'm not a fan is this kind of laisez-faire Python-esque code, but it does satisfy both your comments :)

@cbm755
Copy link
Collaborator Author

cbm755 commented Feb 28, 2019

I forgot to mention a potential downside of this: projects cannot write all their docs using e.g., format long g and then test using

format long g
doctest ...

But I don't know of any project doing that so I don't think we should worry...

Ok to merge?

@cbm755
Copy link
Collaborator Author

cbm755 commented Feb 28, 2019

I've now tested on Matlab, I'll merge now.

@cbm755 cbm755 merged commit 43c4d53 into master Feb 28, 2019
@cbm755 cbm755 deleted the format branch February 28, 2019 20:13
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.

3 participants