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

Use NewXxx functions instead of struct declarations #2835

Closed
rigelrozanski opened this issue Nov 15, 2018 · 7 comments
Closed

Use NewXxx functions instead of struct declarations #2835

rigelrozanski opened this issue Nov 15, 2018 · 7 comments
Labels
good first issue help wanted Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Milestone

Comments

@rigelrozanski
Copy link
Contributor

Throughout the entire sdk we should switch to using NewXxx functions
instead of allowing for struct definitions this will reduce errors as the compiler
will notify us everytime we update a struct but forget to update an object declaration
of that struct type.

@rigelrozanski rigelrozanski added the Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. label Nov 15, 2018
@rigelrozanski rigelrozanski self-assigned this Nov 15, 2018
alessio pushed a commit that referenced this issue Nov 16, 2018
Use stake.NewDescription() to make a new Description - ref #2835
alessio pushed a commit that referenced this issue Nov 21, 2018
- Replace STDIN/STDOUT redirection in `gaiad gentx` with subcommands
  command line options to redirect streams to file since viper does
  not handle redirection well.
- Use `BuildCreateValidatorMsg` to build a `MsgCreateValidator` rather
  than redirecting to `gaiacli tx stake create-validator`.
- `PrintUnsignedStdTx` now takes an `io.Writer` object.
- Mark `--pubkey`, `--amount` and `--moniker` as required flags
  instead of validating them manually.
- Use stake.NewDescription() to make a new Description - ref #2835
cwgoes pushed a commit that referenced this issue Nov 21, 2018
* gaiad gentx subcommands refactoring

- Replace STDIN/STDOUT redirection in `gaiad gentx` with subcommands
  command line options to redirect streams to file since viper does
  not handle redirection well.
- Use `BuildCreateValidatorMsg` to build a `MsgCreateValidator` rather
  than redirecting to `gaiacli tx stake create-validator`.
- `PrintUnsignedStdTx` now takes an `io.Writer` object.
- Mark `--pubkey`, `--amount` and `--moniker` as required flags
  instead of validating them manually.
- Use stake.NewDescription() to make a new Description - ref #2835

* Refresh PENDING.md
@alessio
Copy link
Contributor

alessio commented Dec 6, 2018

I'd propose to adopt the NewXxx naming convention for constructors that return struct pointers, and MakeXxx for those that don't. E.g.:

func NewThing() *Thing {
        return &Thing{}
}

func MakeThing() Thing {
        return Thing{}
}

alessio pushed a commit that referenced this issue Dec 6, 2018
It went lost in last genesis workflow refactoring.

Also address structs initializations as per #2835.
@cwgoes
Copy link
Contributor

cwgoes commented Dec 6, 2018

I'd propose to adopt the NewXxx naming convention for constructors that return struct pointers, and MakeXxx for those that don't.

Do we need to have constructors which return pointers? We should generally avoid using pointers.

@alexanderbez
Copy link
Contributor

It's a case-by-case basis @cwgoes I think. But yes, majority of our types should not return pointers.

@alessio
Copy link
Contributor

alessio commented Dec 6, 2018

...thus let's use the MakeXxx naming convention, shall we?

@cwgoes
Copy link
Contributor

cwgoes commented Dec 6, 2018

...thus let's use the MakeXxx naming convention, shall we?

That's fine with me, but it will require changing a lot of current code. @rigelrozanski thoughts?

@rigelrozanski
Copy link
Contributor Author

rigelrozanski commented Dec 6, 2018

@alessio what to go into the thinking of using MakeXxx instead of NewXxx - I've never seen it before most libraries I see in golang use New.

did a bit of reading on some other's thoughts
https://grokbase.com/t/gg/golang-nuts/13btfsyhy7/go-nuts-what-should-i-name-a-constructor-if-it-does-not-return-a-pointer
https://www.reddit.com/r/golang/comments/9bkql2/stop_returning_errors_in_constructors_use/e53w7l1/

some of this is a little bit beyond me - but I'm not sure I see a good reason to deviate from just using NewXxx

@alessio
Copy link
Contributor

alessio commented Dec 6, 2018

Thanks @rigelrozanski, I thought about it again - we'd better to stick to a single naming convention only for simplicity's sake. Let's go with NewXxx() then 👍

alessio pushed a commit that referenced this issue Dec 11, 2018
It went lost in last genesis workflow refactoring.

Also address structs initializations as per #2835.
@rigelrozanski rigelrozanski removed their assignment Jan 24, 2019
@alessio alessio mentioned this issue Mar 6, 2019
5 tasks
@rigelrozanski rigelrozanski added this to the v1.0 LTS milestone Jun 20, 2019
chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this issue Mar 1, 2024
)

Bumps [github.com/spf13/cast](https://github.com/spf13/cast) from 1.5.1 to 1.6.0.
- [Release notes](https://github.com/spf13/cast/releases)
- [Commits](spf13/cast@v1.5.1...v1.6.0)

---
updated-dependencies:
- dependency-name: github.com/spf13/cast
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue help wanted Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

No branches or pull requests

7 participants