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

Make parent formid a part of autogenerated oid #120

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ezag
Copy link

@ezag ezag commented Oct 22, 2021

Re:

I'm not a native English speaker, so perhaps the docstring wording might be improved.

Copy link
Member

@stevepiercy stevepiercy 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, but I think it would be good to demonstrate both methods of having two forms on a page.

# constructor of both forms. As a result, form's element identifiers
# will inherit parent's ``formid`` and will not overlap.
# (Note: alternatively, we could create an ``itertools.count`` object
# and pass that object as the ``counter`` keyword argument.)
Copy link
Member

Choose a reason for hiding this comment

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

Your English is better than my Russian. 😉 I have only a couple of corrections.

        # We do so by passing distinct ``formid`` keyword arguments to the
        # constructor of both forms.  As a result, the form's element identifiers
        # will inherit the parent's ``formid`` and will not conflict.
        # (Note: alternatively, we could create an ``itertools.count`` object
        # and pass that object as the ``counter`` keyword argument.)

Also after thinking about this, it would be good to demonstrate both examples of having two forms in a page. In other words, (1) let's keep the new method of explicitly providing the formid, and (2) would you please add the alternative example as you describe in your note?

That will require another test for the new widget, but I think it's worth doing.

Copy link
Author

Choose a reason for hiding this comment

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

I think that for an unfamiliar person it would be a bit misleading to have an example with the extra counter related code that could be removed with no apparent effect: setting distinct formids is a must anyway, and altered oids don't seem to matter from the example's standpoint.

That said, it is definitely worth having an example that demonstrates the counter feature; can you think of an appropriate one? And if not, do you think we can consider deprecating and/or removing the counter feature at all, in order to spare further code maintenance efforts?

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