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 methods when there is state. Removed dead code. Default to not export. #16

Closed
wants to merge 6 commits into from

Conversation

ojongerius
Copy link
Contributor

There is a fair amount of getting both values and errors, followed by passing those values to another function.

While going through the code I stumbled on some dead code, which I removed. I also moved the zone helper function out of main (as you planned on doing), and renamed functions that were not used outside of the package to not be exported.

@ojongerius ojongerius changed the title Use methods when there is state. Proposal: Use methods when there is state. Mar 18, 2016
…he package.

Converted some functions to be methods.
* upstream/master:
  Adhere to Go standards.
  Short options where it makes sense.
@ojongerius ojongerius changed the title Proposal: Use methods when there is state. Use methods when there is state. Removed dead code. Default to not export. Mar 18, 2016
@ojongerius
Copy link
Contributor Author

Some extra context/information: I've mainly opened this PR for discussion.

Tests pass but I've not ran any integration tests. Have opened #17 for adding some.

* Can be more compact when only checking for error
* Update doco to reflect method / function renaming
* Fmt
* Refactor GetAllSecrets into a method
@wolfeidau
Copy link
Contributor

Really need to break this into pieces, it is really hard for me to review in it's current state.

  • Yes the ds stuff was a bit of experiment and needs a cleanup, happy to accept that and merge with gusto.
  • On the private stuff, I plan to use these methods in part or full in other tools, hence me keeping most stuff public. A few methods need to be rejigged like that version related one.
  • Zone helper move is welcome
  • Lastly you need to be careful around http://dave.cheney.net/2016/03/19/should-methods-be-declared-on-t-or-t

Again i would love it if you could break these up.

@ojongerius
Copy link
Contributor Author

Cool. this was me being stuck in an airplane and getting a little carried away.

I'll break this up into a few PRs so it's a little more manageable.

@wolfeidau
Copy link
Contributor

@ojongerius no problem at all, totally understand the feeling.

I was tempted to cherry pick but would prefer you just undid the commits and broke it up into bite size chunks to preserve your contributions.

Thanks again.

This was referenced Mar 22, 2016
@ojongerius ojongerius closed this Mar 23, 2016
@ojongerius ojongerius deleted the methods branch March 23, 2016 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants