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

Fixed #21832 #2880

Closed
wants to merge 1 commit into from
Closed

Fixed #21832 #2880

wants to merge 1 commit into from

Conversation

coder9042
Copy link
Contributor

1st commit: Updates code, tests and docs.
2nd commit: Way to create fk instance at command line.

@coder9042
Copy link
Contributor Author

buildbot, test this please.

@coder9042
Copy link
Contributor Author

@timgraham @cjerdonek
I have incorporated the suggestion for adding a way to create fk instance at command line, though there may be some issues with it, kindly help me in identifying those, if any


def create_username_instance(self, model):
inputs = {}
for f in model._meta.fields:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should be model._meta.local_fields. I will change it.

@coder9042
Copy link
Contributor Author

@timgraham
I have updated the PR. Please have a look on my questions on previous diff.
I have not mentioned about overriding in docs as I need answer to my question: Whether we refactor for both or only username?(Currently done for both.)

@timgraham
Copy link
Member

Both places looks fine, but we should make the call to input() part of the method that can be overridden. For a model instance, Chris's use case is to prompt for all the fields of the model, so he won't want the default prompt I believe. I would just message the constructed message as a parameter to the method: get_input_data(self, field, message).

@coder9042
Copy link
Contributor Author

I will add another parameter default=None so that a default value can be passed. Basically, it will be for username, but can be used for other as well now.

@@ -88,19 +88,16 @@ def handle(self, *args, **options):
verbose_field_name = self.username_field.verbose_name
while username is None:
if not username:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to remove this condition as I think that its not required. The while loop already checks username is None so just after that there is no need for if not username.

@coder9042
Copy link
Contributor Author

Tests pass. They fail only in one because of checkout problem.
Also, please look at my comment above on previous diff about removing that extra if-condition.

@coder9042
Copy link
Contributor Author

@timgraham @cjerdonek
If anything else is required here, please tell me.

@cjerdonek
Copy link
Contributor

It's hard for me to evaluate the patch right now without digging into createsuperuser again. From a Django user's perspective, I'll just say it would be nice if the API lets you override the prompting for a single field (and take advantage of the existing implementation) without needing to copy and paste code from the base implementation. I can't tell if that's possible with the current patch or not with get_input_data() handling everything.

@coder9042
Copy link
Contributor Author

@cjerdonek
Yes what you say is possible with this patch.

@cjerdonek
Copy link
Contributor

Okay, if that's the case, then looks good. Thanks a lot for working on this!

@@ -87,20 +87,13 @@ def handle(self, *args, **options):
# Get a username
verbose_field_name = self.username_field.verbose_name
while username is None:
input_msg = capfirst(verbose_field_name) + ' (%s.%s)' % (self.username_field.rel.to._meta.object_name, self.username_field.rel.field_name) if self.username_field.rel else ''
if default_username:
input_msg = "%s (leave blank to use '%s')" % (
Copy link
Member

Choose a reason for hiding this comment

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

use +=

@cjerdonek
Copy link
Contributor

By the way, in my previous comment when I said "looks good," I didn't mean to suggest that I reviewed the patch (which Tim is doing). Thanks, Tim.

@@ -1456,6 +1456,9 @@ it when running interactively.
Use the ``--database`` option to specify the database into which the superuser
object will be saved.

Override ``get_input_data(self, field, message, default=None)`` if you want to
Copy link
Member

Choose a reason for hiding this comment

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

.. versionadded:: 1.8

You can subclass the management command and override get_input_data() if you want to customize data input and validation. Consult the source code for details on the existing implementation and the method's parameters. For example, it could be useful if you have a ForeignKey in REQUIRED_FIELDS and want to allow creating an instance instead of entering an existing instance's primary key.

…FK after 9bc2d76.

Refactored code to allow overriding for custom behaviour.

Thanks Chris Jerdonek and Tim Graham for review.
@coder9042
Copy link
Contributor Author

buildbot, test this please.

if default_username:
input_msg += " (leave blank to use '%s')" % default_username
username_rel = self.username_field.rel
input_msg = force_str('%s%s: ' % (input_msg,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to shorten this, I felt that it would be better to use a variable username_rel = self.username_field.rel rather than using it thrice.

@timgraham
Copy link
Member

merged in 75ff7b8.

@timgraham timgraham closed this Jul 8, 2014
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