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

Convert remaining CharField to TextField #377

Merged
merged 1 commit into from
Nov 12, 2019
Merged

Conversation

dralley
Copy link
Contributor

@dralley dralley commented Nov 11, 2019

closes: #5609
https://pulp.plan.io/issues/5609

Exception, the artifact hashes have a preset length. While we don't gain (or lose) anything from using CharField, it might provide a little tiny amount of extra validation, so I left it as-is.

@dralley dralley force-pushed the field-retype branch 4 times, most recently from 3fb92dc to 7d435cb Compare November 11, 2019 22:37
Copy link
Contributor

@CodeHeeler CodeHeeler left a comment

Choose a reason for hiding this comment

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

Looks like you got all the instances and the relevant docstring sections as well, good job. LGTM.

@@ -5,3 +5,4 @@ ipython
codecov
coverage
flake8
pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to make this change as part of this same PR?

@dralley dralley force-pushed the field-retype branch 3 times, most recently from 86ef34c to 787c5c1 Compare November 12, 2019 18:54
@dralley dralley force-pushed the field-retype branch 2 times, most recently from 4358c0e to 0ed5848 Compare November 12, 2019 20:04
@@ -106,7 +106,7 @@ class ProgressReport(Model):
it will be set to the current task_id.
"""
message = models.TextField()
code = models.CharField(max_length=36)
code = models.TextField()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the max_length here? I thought code was supposed to be a short name for the progress report.

Copy link
Contributor Author

@dralley dralley Nov 12, 2019

Choose a reason for hiding this comment

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

Sure, but how do we decide how short is too short? I feel like we should just make sure plugin writers know they need to keep it short by convention. I can revert it for now though, if it limits plugin writers too much we can increase it or change it.

@dralley dralley merged commit 39d6aa6 into pulp:master Nov 12, 2019
@dralley dralley deleted the field-retype branch November 12, 2019 22:45
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