Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

swap, uint256: move Uint256 implementation to new swarm/uint256 package #2076

Merged
merged 3 commits into from
Jan 15, 2020

Conversation

mortelli
Copy link
Contributor

@mortelli mortelli commented Jan 14, 2020

(this builds on #2063 and adds to it)

This PR:

  • adds a new swarm/uint256 package
  • moves the Uint256 type from swap package to uint256 package
  • adds the etypes alias to go-ethereum/core/types package when it coexists with the swap/types package in a single file discarded change

Since this is the first time I am adding a new package to Swarm, I have opted to do it through an internal PR to have an instance for name changes.

Discussion points:

  • alias choice for go-ethereum/core/types discarded change
  • to reduce verbosity, types could have a short alias, such as t. For now, I have opted not to do this for reasons of code clarity, at the cost of wordiness. discarded change
  • the idea of renaming Uint256 (e.g. to U256) has also been thrown around in the past. This would be a good time to change this, if desired.
    • keep in mind this would also shorten some arguably uncomfortable function names; e.g. Uint64ToUint256 to something like U64ToU256 (although we expect to retire this particular function in the short future)

@mortelli mortelli added ready for review incentives builds on open PR Builds on a PR that is not yet merged. It is blocked until then and the diff won't make sense yet. labels Jan 14, 2020
@mortelli mortelli requested a review from janos January 14, 2020 18:30
@mortelli mortelli self-assigned this Jan 14, 2020
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

We should avoid generalised packages. I think that Uint256 can have its own package with a very small scope, not a general purpose types package as per suggestion from https://github.com/golang/go/wiki/CodeReviewComments#package-names.

@mortelli
Copy link
Contributor Author

We should avoid generalised packages. I think that Uint256 can have its own package with a very small scope, not a general purpose types package as per suggestion from https://github.com/golang/go/wiki/CodeReviewComments#package-names.

alright, this makes sense. but if i name it something like swap/uint256, then i would like to change the function names to avoid redundancy, like so:

types.NewUint256 → uint256.New
types.Uint64ToUint256 → uint256.FromUint64

(and so on)

although the struct itself would be uint256.Uint256, which isn't ideal.

thoughts?

@mortelli mortelli changed the title swap, types: move Uint256 implementation to new swarm/types package swap, uint256: move Uint256 implementation to new swarm/uint256 package Jan 14, 2020
@janos
Copy link
Member

janos commented Jan 14, 2020

I would not mind so much uint256.Uint256 even it is too verbose. There are cases where the only type in a package is named as the package is. One idea that I have in mind is uint256.I like testing.T, but I am not sure if that is better.

@mortelli
Copy link
Contributor Author

I would not mind so much uint256.Uint256 even it is too verbose. There are cases where the only type in a package is named as the package is. One idea that I have in mind is uint256.I like testing.T, but I am not sure if that is better.

alright. then i'm going to merge this to #2063 and if we want to talk about names further, the discussion can take place there.

@mortelli mortelli merged commit 74043d7 into swap-unify-variable-types Jan 15, 2020
@mortelli mortelli deleted the swap-types-package branch January 15, 2020 19:28
@acud acud added this to the 0.5.5 milestone Jan 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
builds on open PR Builds on a PR that is not yet merged. It is blocked until then and the diff won't make sense yet. incentives ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants