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

Fix bug with usage of `before` as array. #7

Merged
merged 3 commits into from Jul 29, 2015

Conversation

@devinivy
Copy link
Member

devinivy commented Jul 12, 2015

Resolves #9.

In exploring topo to write-up some documentation, I discovered a bug when using the before option as an array. I'm submitting this bug fix before I push documentation to the same branch, as I imagine the fix would be higher priority.

A note, I believe this affects hapi when specifying a before option on server.ext.

The problem becomes apparent when expanding the intermediary graph here. Since each entry in graph was originally assigned as an array containing a single array of before groups (here), graphNodeItems.length is always 1 and group ends-up referring to the array of group items rather than an individual group item. Then, when group is used as an object key, the array is coerced to a string, which might look something like 'a,b,c'. This isn't intended, and breaks use of the before option when it specifies multiple groups. The reason this was overlooked is probably because, when before specifies a single group (a common test-case), the array is coerced into the desired string (e.g. ['a'] --> 'a').

I noticed the "explicit" test passed because the before and after options were used redundantly, so I made that test use them each separately. I also added two tests to ensure before and after options can be specified as arrays when performing a simple topological sort.

Docs will follow this in the upcoming days, but can certainly go in a separate PR. Happy to describe the issue and fix in more detail if there are any questions!

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 12, 2015

Can you add a test to hapi showing the bug there?

@devinivy

This comment has been minimized.

Copy link
Member Author

devinivy commented Jul 12, 2015

Yeah, I'll work on it today.

@devinivy

This comment has been minimized.

Copy link
Member Author

devinivy commented Jul 12, 2015

Posted: hapijs/hapi#2642.

By the way, as requested in outmoded/hapi-contrib#33, a requisite comment: I'd love to become lead maintainer of topo.

@devinivy

This comment has been minimized.

Copy link
Member Author

devinivy commented Jul 16, 2015

Should I open issues for the bugs, or will these PRs do the trick?

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 29, 2015

I usually find a PR enough to document a bug.

@hueniverse hueniverse added the bug label Jul 29, 2015
@hueniverse hueniverse self-assigned this Jul 29, 2015
@hueniverse hueniverse added this to the 1.0.3 milestone Jul 29, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 29, 2015

Needs to be rebased...

@devinivy devinivy force-pushed the devinivy:befores-and-docs branch from 3d95faf to b6baf54 Jul 29, 2015
@devinivy

This comment has been minimized.

Copy link
Member Author

devinivy commented Jul 29, 2015

All set– mind taking one last peek?

hueniverse added a commit that referenced this pull request Jul 29, 2015
Fix bug with usage of `before` as array.
@hueniverse hueniverse merged commit e0a38cc into hapijs:master Jul 29, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 29, 2015

Merged. You are all set on permissions. Go ahead and publish 1.0.3. Don't forget to close the milestone and create 1.0.4, and to tag (`git tag -a v1.0.4 -m 'version 1.0.3') it.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 29, 2015

Also, open an issue and PR on hapi to upgrade the shrinkwrap. The issue subject has to follow a specific format (see other 'dependecy' label issues).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.