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

📚 DOCS: Add reference to MySTyc in landing page #413

Merged
merged 2 commits into from
Sep 26, 2021

Conversation

astrojuanlu
Copy link
Contributor

@astrojuanlu
Copy link
Contributor Author

@codecov
Copy link

codecov bot commented Aug 12, 2021

Codecov Report

Merging #413 (048732e) into master (61f1154) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #413   +/-   ##
=======================================
  Coverage   90.67%   90.67%           
=======================================
  Files          14       14           
  Lines        1855     1855           
=======================================
  Hits         1682     1682           
  Misses        173      173           
Flag Coverage Δ
pytests 90.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61f1154...048732e. Read the comment docs.

@chrisjsewell
Copy link
Member

thanks for the cool product @astrojuanlu!

I'd note that higher up the page there is: , or convert existing [RST to Markdown][rst-to-myst], obviously mystrc uses rst-to-myst, so perhaps these two links should be combined, something like RestructuredText conversion: rst-to-myst is a CLI tool which can also be used interactively with mystrc

@astrojuanlu
Copy link
Contributor Author

Thanks @chrisjsewell ! Not sure if this is what you had in mind, WDYT?

Screenshot 2021-08-12 at 18-28-59 MyST - Markedly Structured Text

@chrisjsewell
Copy link
Member

Not sure if this is what you had in mind, WDYT?

yeh I think possibly so thanks.

We could put it lower down, where you had it before, in the "Get started with MyST" panel,
but for me I feel it's quite a nice selling point to people that already have sphinx documentation in RST, so I want it to be more prominent, i.e. higher up the page.

what do you think @choldgraf?

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks good to me - I think it's nice to have it in a prominent spot, though not super invasive.

My one concern is maintenance of the service over time. What are the odds that in 3 months somebody will click the link and the heroku app will be down? If folks don't think that is a significant possibility then I'm +1 on the change

@astrojuanlu
Copy link
Contributor Author

The service is running on my personal Heroku account on the free dyno ( $0.00/month ). Out of the 1000 free dyno hours granted for verified accounts, MySTyc has spent 9.68 hrs this month. I have no intention of taking it down anytime soon.

However, there is a chance someone might abuse the app, or that someday it suffers an orange-site-calypse. If the app exhausts the free hours, my plan is to just scale it down temporarily until the beginning of the following month. If this is not acceptable to folks, here are some ideas:

  • Replace the Python process with JavaScript code, so the computation happens on the client and doesn't need a backend. I don't think https://github.com/executablebooks/markdown-it-myst can do what rst-to-myst can, but I would love to be proven wrong here.
  • Spend some more hours shielding the backend from trivial DoS attacks, for example setting a maximum number of requests by IP or similar. I have no idea what are the best practices about this.
  • Make the costs public and share the load somehow, if they are ever non-zero (unlikely)
  • ?

We could also give the app a domain name, so if it ever moves away from Heroku, the URL is unchanged. It costs money upfront, doesn't solve the underlying issue.

All of this sounds like not very appealing for a weekend hack, but I understand that you want to have some confidence on what you put on the front page. I'm open to suggestions here :)

@choldgraf
Copy link
Member

Hey @astrojuanlu - sorry for the slow response on that latest point. In my opinion, we shouldn't block this PR on battle-testing that heroku app. It's useful and cool, and even if it goes down at some point, at worst people will just open an issue about it, and we can remove the link in the future.

So I think the next step here is to just implement the location swap that @astrojuanlu mentions here: #413 (comment)

and then give it a whirl :-)

@astrojuanlu
Copy link
Contributor Author

No worries @choldgraf ! The swap was already done in the last commit I think :D

@chrisjsewell
Copy link
Member

Yeh that's good for me cheers

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

Sounds great - thanks @astrojuanlu for the improvement!

@choldgraf choldgraf merged commit f362378 into executablebooks:master Sep 26, 2021
@welcome
Copy link

welcome bot commented Sep 26, 2021

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@astrojuanlu astrojuanlu deleted the add-mystyc branch September 27, 2021 05:02
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