Skip to content

feat: Initial draft #143

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mrbazzan
Copy link
Contributor

@mrbazzan mrbazzan commented Apr 16, 2025

Description

Check #150 for details about the issues

Checklist

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

@mrbazzan mrbazzan force-pushed the add_picture_field branch from 9f28f5c to 82dba3a Compare April 29, 2025 11:06
@mrbazzan
Copy link
Contributor Author

mrbazzan commented Apr 29, 2025

@fsbraun
Could you help me check it please?

If I attach the PictureField to a django model e.g

class Test(models.Model):
    image = PictureField()

# create a Test object, then;
test = Test.objects.all()[0]

test.image_ct return the ContentType of the linked image
test.image_fk returns the id of the image

I have an issue with test.image returning None.

@mrbazzan mrbazzan force-pushed the add_picture_field branch from 82dba3a to cfbd392 Compare April 30, 2025 11:52
@mrbazzan
Copy link
Contributor Author

@fsbraun @vinitkumar
I finally figured it out. I think it's ready for the first review now.

@mrbazzan mrbazzan force-pushed the add_picture_field branch from cfbd392 to 0d0fd1c Compare May 6, 2025 10:45
@vinitkumar
Copy link
Member

vinitkumar commented May 7, 2025

Hi @mrbazzan I had a look at your PR. A couple of issues here:

  • The PR description should also be linked to a ticket where it explain what we are trying to achieve here and what is the idea behind the PR. Also, it never hurts to explains what is being done in the PR in the description above so that the reviewer will instantly know what is being done here. It's doesn't have to be an essay but clear description helps a lot in getting code merged faster.
  • Also, with the code, especially when making changes to the existing code or adding new code, please add code comments explain what is done.
  • Also, always check that the CI runs and it's green. In this case, there are some lint errors from ruff and the PR title as well. They also block code getting merged because PR won't get merged with CI failures.

Hope, these are helpful. Thank you.

@mrbazzan
Copy link
Contributor Author

mrbazzan commented May 9, 2025

Thanks a lot for the feedback!
I have created an issue (#150) to briefly explain the goal

  • Also, with the code, especially when making changes to the existing code or adding new code, please add code comments explain what is done.
  • Also, always check that the CI runs and it's green. In this case, there are some lint errors from ruff and the PR title as well. They also block code getting merged because PR won't get merged with CI failures.

Okay. I don't actually expect this to get merged,
I just wanted fsbraun to check if I was heading in the right direction.
(it's more like a draft if anything)

@mrbazzan mrbazzan changed the title Initial draft draft: Initial draft May 9, 2025
@mrbazzan mrbazzan changed the title draft: Initial draft feat: Initial draft May 9, 2025
@mrbazzan mrbazzan force-pushed the add_picture_field branch from 0d0fd1c to c6d0f84 Compare May 9, 2025 10:31
@mrbazzan mrbazzan force-pushed the add_picture_field branch from c6d0f84 to 940392d Compare May 9, 2025 10:37
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