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

Build parameters in composables #441

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ryanhiebert
Copy link

I've been sitting on this work for too long. It's not polished, I haven't updated it in a while, but I need to get it out
for feedback. I hope that it doesn't prove too insulting, and I intend to edit it based on feedback. I think that I was
getting in my own head, afraid of producing something low-quality, but I don't want it to let it stop me from submitting at
all.

It did take a little bit to figure out what compromises I wanted to be able to make with it, and how to work through them.
I think for the most part this is all completely backward compatible. There was one place where I wished I could break
backward compatibility, the second argument to Placeholder. I would prefer that the value be the 2nd positional
parameter, but that would be backward incompatible because format is currently allowed to be a positional argument. All
told, I'd prefer it to be a keyword argument instead. I created a Param class that acts just like Placeholder, but
substituting my preferred constructor API so that you can see what it looks like.

Dictionaries and Sequence parameter accumulation works differently from each other, but I think sanely in both cases. I
don't allow for conflicts, because its a guessing game what that should mean. I do allow for overrides when running the query.

I don't think I've attempted to write any docs just yet, and I do recognize that is important.

Fix #339

@dvarrazzo
Copy link
Member

Hello! Thank you for your contributions

Just a heads up: for a few days I won't be able to review this MR as I'm stuck deeply in a couple of other projects. I'll look into it as soon as I'm relieved of some load.

Thank you very much!

@ryanhiebert
Copy link
Author

Thank you so much for setting my expectations on your scheduling, it makes me feel that my time and work is valued, even if this doesn’t prove to be the right direction for the project.

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.

Build parameters in Composables
2 participants