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

Remove the concept of dry_run #64

Merged
merged 1 commit into from
Apr 1, 2020
Merged

Conversation

mriddle
Copy link
Contributor

@mriddle mriddle commented Mar 31, 2020

The flag was false by default. It adds complexity to the code and can be confused with the other disabled states use_global_uid, the global_uid_options[:disabled] option and global_uid_disabled on the model.

Use global_uid_options[:disabled] if you want to disable the functionality. There's a performance spec for benchmarking included in dc1f998

@mriddle mriddle requested a review from bquorning March 31, 2020 12:36
@mriddle mriddle requested a review from a team as a code owner March 31, 2020 12:36
@mriddle mriddle self-assigned this Mar 31, 2020
@mriddle
Copy link
Contributor Author

mriddle commented Mar 31, 2020

I went to add some notes to a changelog only to realise we don't have one yet. I'll whip one up :)

EDIT: #65 - I will rebase on top of that once it's in master and add a note.

The flag was false by default, adds complexity to the code and can be
confused with the other disabled states `use_global_uid`, the
global_uid_options[:disabled] and `global_uid_disabled` on the model.

Use `global_uid_options[:disabled]` if you want to disable the
functionality. There's a performance spec for benchmarking.
@mriddle mriddle force-pushed the mriddle-remove-dry-run branch from 7bd96a2 to 3d9f44f Compare April 1, 2020 09:14
@bquorning
Copy link
Member

I think we need to check for dry_run somewhere still. Suppose some user has that setting enabled and we just change its behavior – not nice. I think it would be better to raise an exception if dry_run is set.

Copy link
Member

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

👍 if you decide we don’t need to check the dry_run config anyway.

@mriddle
Copy link
Contributor Author

mriddle commented Apr 1, 2020

With the changes prosed here and the others in #63, I was going to suggest that we release these under a major version bump as the validations are also a breaking change

@bquorning bquorning merged commit 325f10e into master Apr 1, 2020
@bquorning bquorning deleted the mriddle-remove-dry-run branch April 1, 2020 11:27
@bquorning bquorning added this to the 4.0.0 milestone Apr 1, 2020
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