-
Notifications
You must be signed in to change notification settings - Fork 50
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
Improve docs for AssignNode; and datamodel.Copy function. #264
Conversation
I'm wondering about the implications of just having a shallow |
Co-authored-by: Mohsin Zaidi <2236875+smrz2001@users.noreply.github.com>
Yeah: we just have to try to solve that with docs docs docs, I think. These are my first try, but maybe you can prose it even better. We could also name this function Fwiw, I don't think I'd be entirely surprised if we also someday want to have some kind of |
Will probably motion to merge this if no objections. I'm always open to the docs getting even better in future edits, but I think this is better than where we started. |
Did have a quick question here @warpfork. For being able to leverage the |
It's always safe to refer to a commit hash by the time it lands on (There's some notes about this policy in HACKME_mergeStrategy.md :)) |
Oh, it's no worry! My PR is still in progress so I can certainly wait till your PR is merged and update the hash. Will keep that document in mind for the future 👍🏼 |
Revisited the docs on the NodeAssembler interface, and described
AssignNode
's contract and purpose better.Also created a
datamodel.Copy
function. This is related because just having that code gave me something to refer to which helped further clarifyAssignNode
. (We should also probably start calling this function from codegen output, and perhaps other places, where similar logic is currently probably repeated, but I've left that out of this diff for now.)