Skip to content

Django 2.1 Upgrades and black formatting#3

Open
mm wants to merge 3 commits intomasterfrom
dj-21
Open

Django 2.1 Upgrades and black formatting#3
mm wants to merge 3 commits intomasterfrom
dj-21

Conversation

@mm
Copy link
Copy Markdown

@mm mm commented Jul 31, 2024

✨ PR Description

Purpose: Update Django Piston codebase to support Django 2.1 by migrating deprecated import paths and applying black code formatter for consistent style.

Main changes:

  • Migrated deprecated Django imports from django.core.urlresolvers to django.urls module for Django 2.1 compatibility
  • Updated ForeignKey fields to include required on_delete=models.CASCADE parameter per Django 2.1 requirements
  • Applied black formatter across entire codebase converting single quotes to double quotes and standardizing whitespace

Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how

Copy link
Copy Markdown

@linearb linearb bot left a comment

Choose a reason for hiding this comment

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

✨ PR Review

The PR introduces Django 2.1 compatibility changes and black formatting. However, there are two critical bugs that will cause runtime failures: Python 3 incompatible base64 decoding and incorrect Django reverse() API usage.

2 issues detected:

🐞 Bug - Using string.decode("base64") which doesn't exist in Python 3 🛠️

Details: The base64 decoding method is incompatible with Python 3. In Python 3, strings don't have a decode() method. This will cause an AttributeError at runtime when HTTP Basic Authentication is used.
File: piston/authentication.py (60-60)
🛠️ A suggested code correction is included in the review comments.

🐞 Bug - Passing fields as the second positional argument (urlconf) instead of as args or kwargs keyword argument 🛠️

Details: The reverse() function is being called with incorrect arguments. The second argument to reverse() is 'urlconf', not 'args'. The old code used permalink() which properly handled the args/kwargs distinction, but this direct call will fail or produce incorrect URLs.
File: piston/emitters.py (287-287)
🛠️ A suggested code correction is included in the review comments.

Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how

Comment thread piston/authentication.py

auth = auth.strip().decode('base64')
(username, password) = auth.split(':', 1)
auth = auth.strip().decode("base64")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐞 Bug - Python 3 Incompatible: Replace with: auth = base64.b64decode(auth.strip()).decode('utf-8') and add import base64 at the top of the file.

Suggested change
auth = auth.strip().decode("base64")
auth = base64.b64decode(auth.strip()).decode('utf-8')
Is this review accurate? Use 👍 or 👎 to rate it

If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over

Comment thread piston/emitters.py

try:
ret['resource_uri'] = reverser(lambda: (url_id, fields))()
ret["resource_uri"] = reverse(url_id, fields)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐞 Bug - Incorrect API Usage: Replace with: ret["resource_uri"] = reverse(url_id, args=fields) if fields is a tuple/list, or ret["resource_uri"] = reverse(url_id, kwargs=fields) if fields is a dict. Check what resource_uri() returns to determine the correct parameter name.

Suggested change
ret["resource_uri"] = reverse(url_id, fields)
ret["resource_uri"] = reverse(url_id, args=fields)
Is this review accurate? Use 👍 or 👎 to rate it

If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over

@mm
Copy link
Copy Markdown
Author

mm commented Jan 23, 2026

wow, blast from the past ;p

@prateekmadhikar
Copy link
Copy Markdown

wow, blast from the past ;p

@mm
Copy link
Copy Markdown
Author

mm commented Jan 23, 2026

image

@mgingras mgingras removed their assignment Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants