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

[#5] added Logux.undo shortcut #13

Merged
merged 6 commits into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@markfrst
Contributor

markfrst commented Sep 29, 2018

No description provided.

@coveralls

This comment has been minimized.

coveralls commented Oct 2, 2018

Pull Request Test Coverage Report for Build 110

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 98.904%

Totals Coverage Status
Change from base Build 103: 0.006%
Covered Lines: 361
Relevant Lines: 365

💛 - Coveralls
@@ -55,6 +55,16 @@ def self.add(type, meta: {})
logux_add.call(type, meta: logux_meta)
end
def self.undo(meta = {}, reason = nil)

This comment has been minimized.

@wilddima

wilddima Oct 3, 2018

Member

Let's use keyword arguments, and instead of {} as default meta value, let's use Logux::Meta.new({})

This comment has been minimized.

@markfrst

markfrst Oct 3, 2018

Contributor

okay

This comment has been minimized.

@markfrst

markfrst Oct 3, 2018

Contributor

@wilddima fixed

it 'makes request' do
subject
expect(WebMock).to have_requested(:post, Logux.configuration.logux_host)

This comment has been minimized.

@dsalahutdinov

dsalahutdinov Oct 3, 2018

Collaborator

Here is new implemented matcher to check request is made

This comment has been minimized.

@markfrst

markfrst Oct 3, 2018

Contributor

👍

This comment has been minimized.

@markfrst
@@ -55,6 +55,16 @@ def self.add(type, meta: {})
logux_add.call(type, meta: logux_meta)
end
def self.undo(meta = Logux::Meta.new({}), reason = nil)
logux_add = Logux::Add.new
logux_meta = Logux::Meta.new(meta.merge(status: 'processed'))

This comment has been minimized.

@wilddima

wilddima Oct 8, 2018

Member

@markfrst I asked @ai about it, and we don't need processed here, so let's remove this row

This comment has been minimized.

@markfrst

markfrst Oct 8, 2018

Contributor

okay

@@ -55,6 +55,16 @@ def self.add(type, meta: {})
logux_add.call(type, meta: logux_meta)
end
def self.undo(meta = Logux::Meta.new({}), reason = nil)

This comment has been minimized.

@wilddima

wilddima Oct 8, 2018

Member

And we lost keyword argument here 😉

markfrst added some commits Sep 29, 2018

fixes after review
fixed specs and other minor changes
@wilddima

@markfrst great, thank you!

@wilddima wilddima merged commit 3fdd2c8 into logux:master Oct 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.006%) to 98.904%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment