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.

Sign up for GitHub

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

Jump to bottom

Improve populate command by handling new fields in models #124

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Improve populate command by handling new fields in models #124

wants to merge 1 commit into from

Conversation

KwikKill
Copy link
Member

@KwikKill KwikKill commented Jan 4, 2024 •
edited
Loading

Small changes but still useful to test with empty DB.

@KwikKill KwikKill requested a review from Lymkwi January 4, 2024 23:05
@KwikKill KwikKill self-assigned this Jan 4, 2024
Lymkwi
Lymkwi reviewed Jan 10, 2024
View reviewed changes
Copy link
Contributor

@Lymkwi Lymkwi left a comment

Choose a reason for hiding this comment

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

Only small concern, and I've got to run it once for tests. Otherwise, so far so good.

insalan/tournament/management/commands/populate.py
maxTeam=randint(1, 10),
logo=self.generate_logo(),
is_announced=randint(0, 100) < 50,
player_price_online=randint(0, 50),
Copy link
Contributor

Choose a reason for hiding this comment

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

prices are not int, they are Decimal. Are you sure the conversion is done when this is assigned?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was in my testing but it could be better for testing to have Decimal, i will change it soon 👍

Lymkwi
Lymkwi suggested changes Feb 15, 2024
View reviewed changes
Copy link
Contributor

@Lymkwi Lymkwi left a comment

Choose a reason for hiding this comment

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

  • there was a comment about decimals
  • you should probably fix the generation of random stuff in the tournament name, apparently it's breaking length constraints at the moment?

@KwikKill
Copy link
Member Author

  • there was a comment about decimals
  • you should probably fix the generation of random stuff in the tournament name, apparently it's breaking length constraints at the moment?
  • No commits were added since last review, this will wait after the LAN. The change to decimal is planned and has not been forgotten.
  • The name constraint is not from this part of code. This was fixed in a0f468e but this branch was not rebased yet as this is not the priority.

@KwikKill KwikKill marked this pull request as draft October 4, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lymkwi Lymkwi Lymkwi requested changes

Assignees

@KwikKill KwikKill

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KwikKill @Lymkwi