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

Implement echo=None functionality via run-as-program #21

Merged
merged 2 commits into from
Nov 28, 2022

Conversation

hugetim
Copy link
Contributor

@hugetim hugetim commented Oct 12, 2022

Addresses #16, supersedes #15 and #18.

@hugetim
Copy link
Contributor Author

hugetim commented Oct 12, 2022

I'm not currently aware of any valid Stata code this can't handle. In particular, #delimit statements actually seem to be OK within program definitions, as far as I can tell (and it's no longer a program for my run-as-program code due to your clean_code converting the delimiters). But there are two output quirks:

  1. The run-as-program workaround generates a blank line in the output (for each multi-line block of code it is used for).
  2. Multi-line mata programs are fully echoed. Otherwise, it seems they would show no output at all.

@hugetim
Copy link
Contributor Author

hugetim commented Nov 7, 2022

Just to follow up on this, do you have an estimate on when you might have time to review this pull request? I'm looking to roll this out to others in my organization, and I'm wondering whether I should have them install from my own fork or wait to do it more cleanly via pip install pystata-kernel. Thanks! (I really appreciate your volunteered efforts here and sincerely don't want to come across as demanding or rushing you.)

@hugetim
Copy link
Contributor Author

hugetim commented Nov 21, 2022

For anyone who sees this, I have now created a separate project which implements this pull request (after reorganizing the code) and makes other improvements: https://github.com/hugetim/nbstata

@ticoneva
Copy link
Owner

My apology for being absent for so long. Sickness and work have kept me from coming online. I have merged your request. I am also thinking of how to make this project sustainable so that one person's sickness will not bring updates to a halt. I could perhaps add you as a collaborator?

@ticoneva ticoneva merged commit f00954a into ticoneva:main Nov 28, 2022
@hugetim
Copy link
Contributor Author

hugetim commented Nov 29, 2022

Thanks! I would like to collaborate but I'm not sure the best way to go about it. I think I've made a number of improvements to my version in the past week or so. But they would be hard to port over to pystata_kernel because I've ended up doing a lot of refactoring, as well as restructuring the code to work as an nbdev project (something I've been wanting to try out for a while). Would you like to be a collaborator on nbstata? Or what do you think?

@hugetim
Copy link
Contributor Author

hugetim commented Nov 29, 2022 via email

@hugetim
Copy link
Contributor Author

hugetim commented Nov 29, 2022

To be more specific about the fixes and features I've added in nbstata, looking back:

  • Made #delimit persist across cells
  • Added some automatic tests, with Github Actions CI, using nbdev. (Only stuff that doesn't use Stata can be tested in the cloud, but I've also added some semi-automatic Stata tests in the code notebooks that I can run locally.)
  • Clarified the install instructions
  • Improved _run_as_program (which originated in this pull request) to remove the extra line and clean itself up properly in case of a Stata code error
  • Added an echo magic
  • Refined clean_code (renamed to standardize_code) to handle some edge cases that came up during testing

I've been super-fortunate to have some free time at my job recently during which I've been able to work on this stuff on the clock. But that is unlikely to last for long.

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.

2 participants