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

Add a Contributing section to the docs, and cite it in error message #158

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sideshowbarker
Copy link
Contributor

This change adds a Contributing section to the README.md, and cites it from the error message we emit when a string is passed to Rope≫Append() that’s larger than the allowed string size. Relates to #153, as an alternative to #157.

This change adds a Contributing section to the README.md, and cites it
from the error message we emit when a string is passed to Rope≫Append()
that’s larger than the allowed string size.

Relates to #153.
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This is super-great. I have some editorial nits I'd be happy to correct myself (e.g. use more code font), but at a high-level my only question is:

  • Should we try to document AsString vs. ExtractAll() vs. ... any other relevant patterns? That's what took me a long time to figure out when doing 3b11b2d . I just assumed "oh, strings are good, let me use AsString" and that turned out to be a terrible idea.

  • Should we be more explicit about how to convert between ropes and cutropes? (It seems like the general pattern is, use Scratch?)

Your code examples use ExtractAll(), but say "happens to be a cutrope". I think what I found over the course of recent work is, some parts of Wattsi really really expect ropes, not strings, so in general you should be paying a lot of attention to input types, and know clearly how to convert between cutropes (a common return value) and ropes (a common argument type).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants