Skip to content
This repository has been archived by the owner on Nov 23, 2024. It is now read-only.

Remove unused and often misleading options #68

Merged
merged 2 commits into from
Apr 2, 2020

Conversation

mriddle
Copy link
Contributor

@mriddle mriddle commented Apr 1, 2020

In the following cases we were passing options around but never using them, leftover from a previous implementation but never used. I found them misleading as whatever you passed in would ultimately be ignored.

I've removed them so the code is more explicit and hopefully a little less confusing.

@mriddle mriddle requested a review from a team as a code owner April 1, 2020 15:02
@mriddle mriddle self-assigned this Apr 1, 2020
@mriddle mriddle force-pushed the mriddle-remove-unused-options branch from 70f9348 to c639309 Compare April 1, 2020 15:04
@bquorning bquorning added this to the 4.0.0 milestone Apr 1, 2020
@bquorning
Copy link
Member

Sorry for being pedantic, but could we possibly split this into two commits: One that starts using self.global_uid_options instead of passed-in options, and one removing arguments and parameters?

@mriddle
Copy link
Contributor Author

mriddle commented Apr 2, 2020

@bquorning Sure thing!

EDIT: You're not being pedantic by the way, I appreciate the fresh set of eyes and understand where you're coming from. In one commit it was unclear that the options were unused. In two commits, it's easier to follow my logic.

@mriddle mriddle force-pushed the mriddle-remove-unused-options branch from c639309 to 70b1957 Compare April 2, 2020 11:30
The options passed in are just the `global_uid_options`, refer to them
directly to make the code easier to follow.
@mriddle mriddle force-pushed the mriddle-remove-unused-options branch from c6fd83c to 9f51314 Compare April 2, 2020 11:53
In the following cases we were passing options around but never using
them, introduced in the original implementation but never used. I found
them misleading as whatever you passed in would ultimately be ignored.

I've removed them so the code is more explicit and hopefully a little
less confusing.
@bquorning bquorning merged commit abf02d3 into master Apr 2, 2020
@bquorning bquorning deleted the mriddle-remove-unused-options branch April 2, 2020 11:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants