Add jump_url field to infraction model #824
Conversation
✅ Deploy Preview for pydis-static ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
shtlrs
left a comment
shtlrs
left a comment
There was a problem hiding this comment.
This is purely a code review, will test later!
Thanks for the PR vivek ! :D
| ('api', '0085_infraction_jump_url_text'), | ||
| ] | ||
|
|
||
| operations = [ |
There was a problem hiding this comment.
Is this file supposed to exist ?
It looks like it came from a merge, but it's strange since the two dependencies have the same identifier.
I think deleting these 2 & making a new migration would be cleaner
There was a problem hiding this comment.
This file's the result of running python manage.py makemigrations --merge.
There was a problem hiding this comment.
Ah I see, I didn't know this existed.
Don't you think it'd be cleaner to have a separate one, with a different id ? instead of this merged format ?
There was a problem hiding this comment.
Similar merged migration files already exist in the project so imo it's fine.
There was a problem hiding this comment.
Sure, that's one way to view this.
Even though I don't fully agree, since having something that can or could've been improved doesn't mean we should do it again.
I think the conflict happened because of the migration I did on the nominations table, which has already been merged into main 2 days ago.
And the conflict happened just on the naming, and not on the model itself, and I still think it should be done on its own.
We can wait for others to see what they think about this.
There was a problem hiding this comment.
Migration files are not for humans anyway. However, I've gone ahead and generated a new migration file.
| help_text="Whether a DM was sent to the user when infraction was applied." | ||
| ) | ||
|
|
||
| jump_url_text = models.CharField( |
There was a problem hiding this comment.
Wouldn't it be better to store the raw URL in the database ? And do the formatting on the bot's side ?
There was a problem hiding this comment.
I was initially doing what you described but changed to this because it looked cleaner bot-side. But I'll do what you're suggesting because it will be more modular.
wookie184
added
area: API
vivekashok1221
force-pushed
the
vivek/add-jump-url-field
branch
from
9efa584 to
8a95402
Compare
vivekashok1221
added
s: needs review
vivekashok1221
changed the title
|
|
||
| jump_url = models.CharField( | ||
| default='', | ||
| max_length=88, |
There was a problem hiding this comment.
This is pretty tight, I can't think of any valid reason it should be longer than this so I guess it's fine though...
| ) | ||
|
|
||
| jump_url = models.CharField( | ||
| default='', |
There was a problem hiding this comment.
What was the reason behind using an empty string here, rather than making it nullable?
There was a problem hiding this comment.
Replied here: python-discord/bot#2372 (comment)
There was a problem hiding this comment.
I've now replaced CharField with URLField and added null=True in ed35776.
Closes #801 .
Related bot issue: python-discord/bot#2329
Related bot PR: python-discord/bot#2372