andrewheiss / SublimeStataEnhanced

Plugin that adds support for Stata 11–15 for Sublime Text 2 and 3
56 stars 22 forks source link

"StataMP" hardcoded #43

Closed elbersb closed 8 years ago

elbersb commented 8 years ago

With this commit https://github.com/andrewheiss/SublimeStataEnhanced/commit/c362a721958370657909745cb51c986722debcbb, "StataMP" is now hardcoded when running the complete do-file. Sorry, my fault! Is it possible to get the value from the user's settings? If yes, I can change this.

andrewheiss commented 8 years ago

Oh, oops! I should have paid more attention to the pull request :)

No, there's not a way to use the setting from the .sublime-settings file in the build file, since it's a JSON file and doesn't get compiled and run like the rest of the Python code. Build files only support these variables.

I had originally made the build file run the .do file with Finder to (1) maintain support for Stata 11, since it doesn't have AppleScript support, and (2) allow the whole file to be run, avoiding the 8191 character issue.

Sublime Text does let you have multiple build systems, though (see the "Variants" section here). Maybe you could do this:

  1. Create a variant that runs the .do file with the former Finder command ("tell application \"Finder\" to activate & open POSIX file \"$file\"")
  2. Create a variant that runs the .do file with Stata's fancy AppleScript ("DoCommandAsync \"do \\\"$file\\\"\" with addToReview")
  3. Add instructions to the README about editing the build file with the correct Stata name

I just went down an AppleScript rabbit hole to see if there's a version-agnostic way to open Stata with AppleScript, like tell application Stata or tell application Stata*, and the closest solution I've found is to reference the application's bundle identifier, which you can find using plutil -p /Applications/Stata/StataSE.app/Contents/Info.plist | grep BundleIdentifier (see here).

With that identifier, you can run do stuff like tell application id "com.stata.stata14" to activate in AppleScript and open the default version of Stata without having to worry about SE vs. MP. However, this only fixes the issue of different flavors of Stata within a version, not across versions (i.e. using com.stata.stata14 will not work if someone has Stata 13, 12, etc.). So using the bundle id would fix the MP/SE issue, but not the version issue.

Hrm.

elbersb commented 8 years ago

Another idea might be to check for all possible Stata versions with code such as this:

try
    tell me to get application id "com.stata.stata13"
    set appExists to true
on error
    set appExists to false
end try

This way we could support both Stata 11 and newer versions. This could also be implemented in the main plugin, so the user no longer has to enter the Stata version by themselves.

What do you think? I could work on this if you think it's a good idea.

andrewheiss commented 8 years ago

Yeah, something like that would work, moving backwards from 14 so that the most recent Stata gets run (if they have multiple versions installed). The only situation where that would be bad is if someone has 13 and 14 installed and they're using 13 as the default for whatever reason, but that seems like a super edge case.

Go for it! :)

elbersb commented 8 years ago

Okay, here's something. I want to change a few more things before I send a pull request, but you should have a look first at how many things I broke in the process :)

There's a new method that detects the Stata version. (Note that it's still possible to define the application name oneself.) The build system now uses a Sublime command that is defined in the python file. I think overall this is much cleaner.

https://github.com/elbersb/SublimeStataEnhanced/commit/2f53650dbd736e05cf1b22a8da064f203339b24f

andrewheiss commented 8 years ago

Cool cool cool. It looks great in theory. My Stata license just expired, so I can't test it for a while (waiting for my adviser to get back in the country so she can pay for a new one :) ), so I'll just trust that it works? (in all honesty, that's how I've been testing this whole plugin—if it works on my computer, I assume it works everywhere until someone opens an issue. Truly following best practices here.)

So test it a bunch, make sure it works, and then PR it.

elbersb commented 8 years ago

Sounds like a good plan :) Here's another commit: https://github.com/elbersb/SublimeStataEnhanced/commit/76104da5c6ee75b968f85aab210a035c62733c9d

This one get rids of the two different commands for Stata > 12 and Stata <=12. This way, the build system should also work on windows. I think this is more cleaner and maintainable way to organize the code, because all the logic is in one file. Or was there a good reason to separate the two commands? I hope this all works, but of course I can only test it with Stata 13 on OS X. If you like this today, I will PR it together, and then we hope that there are not too many bugs.

andrewheiss commented 8 years ago

Oh cool, that looks great! There wasn't a reason for separating the two different commands—it just evolved that way. Make a PR!

elbersb commented 8 years ago

Okay, here is the pull request. I think this makes the code more maintainable in the long run, I just hope that there aren't too many bugs in there.

andrewheiss commented 8 years ago

Awesome, thanks! I just merged it in and made a new release, so we'll see how many problems start showing up. Having some sort of Travis-CI or at least some way of testing packages would be super useful someday…

elbersb commented 8 years ago

Great! I just updated to the new version, and in the message that pops up it says: "If your version of Stata is not named "StataSE", follow the instructions in the README file so that this plugin will work properly." Could you get rid of that? Might be confusing for new users.

andrewheiss commented 8 years ago

Ah, forgot about that install message. Fixed now.