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

Remove all Context.Global* methods #410

Merged
merged 7 commits into from
May 18, 2016
Merged

Remove all Context.Global* methods #410

merged 7 commits into from
May 18, 2016

Conversation

meatballhat
Copy link
Member

and change the behavior of the non-Global variants to always search up the
context lineage.

Closes #385

and change the behavior of the non-Global variants to always search up the
context lineage.

Closes #385
@codegangsta codegangsta added the status/claimed someone has claimed this issue label May 16, 2016
@meatballhat meatballhat added kind/feature describes a code enhancement / feature request and removed status/claimed someone has claimed this issue labels May 16, 2016
lineage := []*Context{}
cur := c

for {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like you could shrink this up by doing something like:

for cur := c; cur != nil; cur = cur.parentContext {
  lineage = append(lineage, cur)
}

@jszwedko
Copy link
Contributor

👏 I'm super happy to see this.

Do you think you could add a couple of tests testing:

  • context.Lineage
  • That the context accessor functions properly traverse up (or maybe just test lookupFlagSet since that is where most of the logic is)
  • That IsSet traverses up

❤️

@meatballhat meatballhat added this to the v2.0.0 milestone May 17, 2016
setFlags map[string]bool
globalSetFlags map[string]bool
parentContext *Context
App *App
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 This reduced signature feels much better.

@jszwedko
Copy link
Contributor

Beautiful.

@jszwedko jszwedko merged commit 9eaa109 into v2 May 18, 2016
@jszwedko jszwedko deleted the remove-global-lookups branch May 18, 2016 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature describes a code enhancement / feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants