-
Notifications
You must be signed in to change notification settings - Fork 78
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
Adding debounce time to avoid spamming changelog creation #87
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea. Thanks!
But we definitely need tests)
@@ -205,6 +244,11 @@ class <%= @migration_class_name %> < ActiveRecord::Migration<%= ActiveRecord::VE | |||
IF history_limit IS NOT NULL AND history_limit = size THEN | |||
NEW.log_data := logidze_compact_history(NEW.log_data); | |||
END IF; | |||
|
|||
IF debounce_time IS NOT NULL THEN | |||
NEW.log_data := logidze_merge_recent(NEW.log_data, debounce_time); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, it's better to handle debounce before adding a new version instead of adding a new first version first and than merging it with the previous.
So, on line 231 we can do:
IF debounce_time IS NOT NULL AND version->>'ts'::bigint - NEW.log_data#>'{h,-1,ts}')::text::bigin <= debounce_time
# merge new version with the previous one
version = logidze_merge_version(NEW.log_data#>'{h,-1}', version)
# remove the previous version from log
NEW.log_data = jsonb_set(
NEW.log_data,
'{h}',
NEW.log_data->'h' - (size -1 )::text
);
END
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the brilliant suggestion, I made the change accordingly and added some tests
(version->>'ts')::bigint - (NEW.log_data#>'{h,-1,ts}')::text::bigint <= debounce_time | ||
) THEN | ||
-- merge new version with the previous one | ||
version := NEW.log_data#>'{h,-1}' || version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this step?
We're merging changesets on the next line.
The only key that could me missing in the new version is the meta key ("m"
); but I think merging meta information could lead to undesired behaviour: for example, if we track responsibility than we should care only about the last meta information (and its absence).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will merge the v
, ts
, (meta) m
and (responsible_id) r
along with the changesets c
so we always get the latest ones. However, ||
can't do the deep merge for the changeset so we need to do it separate in the next line.
For meta m
, we can do a deep merge but just like you said, I don't think it's necessary
@@ -10,6 +10,8 @@ class ModelGenerator < ::ActiveRecord::Generators::Base # :nodoc: | |||
|
|||
class_option :limit, type: :numeric, optional: true, desc: "Specify history size limit" | |||
|
|||
class_option :debounce_time, type: :numeric, optional: true, desc: "Specify debounce time" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a measurement unit to the description
@zocoi Let's add a changelog entry and update the readme with the new feature description. And we're almost done) |
@palkan @charlie-wasp I changed the version number logics when merging logs, I kept the previous version number for consistency. Previously Thanks |
CHANGELOG.md
Outdated
@@ -2,6 +2,33 @@ | |||
|
|||
## master | |||
|
|||
## 0.8.0 (2018-10-01) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave the change log entry under master
section. We don't know the date and version of the release this feature will be included)
CHANGELOG.md
Outdated
@@ -2,6 +2,33 @@ | |||
|
|||
## master | |||
|
|||
## 0.8.0 (2018-10-01) | |||
|
|||
- [PR [#87](https://github.com/palkan/logidze/pull/87)] Adding debounce time to avoid spamming changelog creation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add your Github handle here (and in the bottom of the document)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, it should be done
CHANGELOG.md
Outdated
|
||
without `debounce_time` | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add js
lang attributes here and in the snippet below to make RuboCop ignore these snippets (right it considers them as Ruby code)
Great job! Thanks! Will release a new version soon |
Usecase: We have a
Blog
model which get frequent minor updates for thecontent
column as the editor keeps typing with the CMS autosave feature (every 5s) and we don't want to create log to capture tiny changes. The content text could be big so we need to save space. I came up with a concept similar to https://underscorejs.org/#debounce. The idea is that if the several logs came in within a time period, we only keep the latest one and discard the rest.becomes
Notice that version 2 is gone
Please give feedback and help optimize the sql part.
Right now the code doesn't care if columns are edited by different
responsible_id
s but that could be easily added.