-
Notifications
You must be signed in to change notification settings - Fork 57
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
add support for Stata 17 graphs #394
Conversation
@@ -299,7 +299,7 @@ def expect(self, text, child, md5, text_to_exclude=None, display=True): | |||
error_re = r'^r\((\d+)\);' | |||
|
|||
# The minimum linesize in Stata is 40 characters | |||
g_exp = r'\(file {}'.format(self.cache_dir_str[:34]) | |||
g_exp = r'\(?file {}'.format(self.cache_dir_str[:34]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably good to add a comment above this noting that the (
doesn't exist in Stata 17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @kylebarron , I've added a comment in 97709b6
@@ -325,6 +329,9 @@ def expect(self, text, child, md5, text_to_exclude=None, display=True): | |||
continue | |||
if match_index == 2: | |||
g_path = [self.expect_graph(child, child.match.group(0))] | |||
if g_path[0] is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simon-d-s Sorry I've been MIA. Can this be fixed by instead adding ^
at the start of g_exp
? Instead of the logic here and the if else
logic below, add ^
at the start of g_exp
so that the messages for a file not existing and the file being saved are not confused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcaceresb No problem.
For Stata 17 it would work, but I cannot see how it would work for Stata 16.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simon-d-s I tested it out on this branch and it was working for me on 15 and 16. I don't have 17 though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcaceresb Sorry, I did not fully understand what you mean - with the code it's clear now.
Unfortunately, this will not work on Stata 17. As you can see in the following example, the beginnings of the messages for a file not existing and the file being saved differ only by the (
:
. sysuse auto
(1978 automobile data)
. hist price
(bin=8, start=3291, width=1576.875)
. graph export foobar.png, replace
(file foobar.png not found)
file foobar.png saved as PNG format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simon-d-s Ah, I see what you mean. I'll try to review your proposed fix when I get a sec. These weeks are a bit full for me, though.
Implemented changes made in kylebarron#394 for inline graph capabilities in Stata 17.
This reverts commit a3c7f87.
git diff upstream/master -u -- "*.py" | flake8 --diff