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

UDict + cmp_generator #3

Merged
merged 30 commits into from
Jun 2, 2024
Merged

UDict + cmp_generator #3

merged 30 commits into from
Jun 2, 2024

Conversation

bleudev
Copy link
Member

@bleudev bleudev commented May 22, 2024

Checklist:

closes #1
closes #2
closes #7
closes #8

@bleudev bleudev added new feature New feature new class New class labels May 22, 2024
@bleudev bleudev self-assigned this May 22, 2024
@bleudev bleudev marked this pull request as ready for review May 22, 2024 19:12
@bleudev bleudev marked this pull request as draft May 22, 2024 19:12
@bleudev bleudev marked this pull request as ready for review May 22, 2024 19:16
@bleudev bleudev marked this pull request as draft May 22, 2024 19:16
@bleudev bleudev mentioned this pull request May 22, 2024
@bleudev bleudev added this to the 1.0 milestone May 27, 2024
@bleudev bleudev added the new function New function label May 27, 2024
@bleudev bleudev mentioned this pull request May 29, 2024
3 tasks
@bleudev bleudev marked this pull request as ready for review June 1, 2024 18:42
Copy link
Member

@moontr3 moontr3 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@moontr3 moontr3 left a comment

Choose a reason for hiding this comment

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

I'd change some things honestly:

  • Make indices start at 0 instead of 1
  • Getting the default attribute in the UDict.get(default=0) function, not when the UDict is initialised (eg. UDict(default=0))

@bleudev
Copy link
Member Author

bleudev commented Jun 2, 2024

I'd change some things honestly:

  • Make indices start at 0 instead of 1
  • Getting the default attribute in the UDict.get(default=0) function, not when the UDict is initialised (eg. UDict(default=0))
  1. Not. It's feature
  2. I'll do this

@bleudev bleudev requested review from moontr3 and TheAihopGG June 2, 2024 10:10
Copy link
Member

@moontr3 moontr3 left a comment

Choose a reason for hiding this comment

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

I'd remove the default kw arg from the UDict.__init__(), since if you want to pass in a key named default, you'll need to do that using a dict instead of keyword args, which can lead to bad code readability.

And I'd really change the indices to start from 0 for convenience.

Other than that everything looks good.

@bleudev
Copy link
Member Author

bleudev commented Jun 2, 2024

I'd remove the default kw arg from the UDict.__init__(), since if you want to pass in a key named default, you'll need to do that using a dict instead of keyword args, which can lead to bad code readability.

And I'd really change the indices to start from 0 for convenience.

Other than that everything looks good.

No, i don't want change this things

@bleudev bleudev requested a review from moontr3 June 2, 2024 11:42
Copy link
Member

@moontr3 moontr3 left a comment

Choose a reason for hiding this comment

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

You do you then

@bleudev
Copy link
Member Author

bleudev commented Jun 2, 2024

You do you then

Thanks for approval!

@bleudev bleudev removed the request for review from TheAihopGG June 2, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new class New class new feature New feature new function New function
Projects
None yet
3 participants