greg/quoracle
1
0
Fork 0
Code Issues 1 Pull requests 1 Projects Releases Packages Wiki Activity Actions

Make Node a dataclass #5

Open
adsharma wants to merge 1 commit from adsharma/dataclass into main
pull from: adsharma/dataclass
merge into: greg:main
Conversation 3 Commits 1 Files changed 1 +17 -27
adsharma commented 2021-03-31 19:29:39 +00:00 (Migrated from github.com)
Copy link

Dataclasses make the code more compact and generate many convenience
functions.

Dataclasses make the code more compact and generate many convenience functions.
adsharma (Migrated from github.com) reviewed 2021-03-31 19:30:41 +00:00
quoracle/quorum_system.py
adsharma (Migrated from github.com) commented 2021-03-31 19:30:41 +00:00
Copy link

This code would not be necessary if we made:

read_capacity: float = 1.0

in the dataclass. More instances below.

This code would not be necessary if we made: ``` read_capacity: float = 1.0 ``` in the dataclass. More instances below.
mwhittaker commented 2021-04-05 17:01:52 +00:00 (Migrated from github.com)
Copy link

Ah, I've been targeting Python 3.6, and I think dataclasses are a 3.7+ thing?

Ah, I've been targeting Python 3.6, and I think dataclasses are a 3.7+ thing?
adsharma commented 2021-04-05 18:45:00 +00:00 (Migrated from github.com)
Copy link

Backports are available: https://pypi.org/project/dataclasses/

In general, dataclasses eliminate so much boilerplate code that I use them wherever I can.

Backports are available: https://pypi.org/project/dataclasses/ In general, dataclasses eliminate so much boilerplate code that I use them wherever I can.
mwhittaker (Migrated from github.com) reviewed 2021-04-11 01:42:53 +00:00
mwhittaker (Migrated from github.com) left a comment
Copy link

Thanks for the PR! I spent some time reading up on dataclasses, and I think the switch here may not be worth it.

  • I don't think we benefit much since there's not a lot of boilerplate to remove in this case.
  • The dataclass also introduces capacity into Node which it didn't previously have, and it's not clear what capacity means. For example, if we construct a node like Node('a', read_capacity=100, write_capacity=200), there's not a nice value for capacity.
  • Also since Node is part of the Expr hierarchy, it feels weird to me that Node would be a dataclass but not And or Or.
Thanks for the PR! I spent some time reading up on dataclasses, and I think the switch here may not be worth it. - I don't think we benefit much since there's not a lot of boilerplate to remove in this case. - The dataclass also introduces `capacity` into `Node` which it didn't previously have, and it's not clear what `capacity` means. For example, if we construct a node like `Node('a', read_capacity=100, write_capacity=200)`, there's not a nice value for `capacity`. - Also since `Node` is part of the `Expr` hierarchy, it feels weird to me that `Node` would be a dataclass but not `And` or `Or`.
quoracle/expr.py
mwhittaker (Migrated from github.com) commented 2021-04-11 01:34:58 +00:00
Copy link

This should be __post_init__.

This should be `__post_init__`.
adsharma commented 2021-04-12 20:54:52 +00:00 (Migrated from github.com)
Copy link

Yes - it saves only 10 lines in this diff. Not an earth shattering difference :)

I've addressed the bugs you found in the code and simplified the typing and default values.

I'll leave it here just in case you end up using dataclasses elsewhere and then want to convert this one for consistency.

Yes - it saves only 10 lines in this diff. Not an earth shattering difference :) I've addressed the bugs you found in the code and simplified the typing and default values. I'll leave it here just in case you end up using dataclasses elsewhere and then want to convert this one for consistency.
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin adsharma/dataclass:adsharma/dataclass
git checkout adsharma/dataclass

Merge

Merge the changes and update on Forgejo.
git checkout main
git merge --no-ff adsharma/dataclass
git checkout main
git merge --ff-only adsharma/dataclass
git checkout adsharma/dataclass
git rebase main
git checkout main
git merge --no-ff adsharma/dataclass
git checkout main
git merge --squash adsharma/dataclass
git checkout main
git merge --ff-only adsharma/dataclass
git checkout main
git merge adsharma/dataclass
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something isn't working

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
question

Further information is requested

  
wontfix

This will not be worked on

No labels
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: greg/quoracle#5
No description provided.
Delete branch "adsharma/dataclass"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?