Initial design

[martonpe]

The original idea:

CSV2Avro.logger.event("finished_converting", filename: filename, schema: schema)
                 .monitored("Finished converting #{filename}", "Detailed description")
                 .metric("lines_processed", 600, type: 'counter')
                 .metric("bad_rows", 100, type: 'counter')
                 .metric("conversion_time", 600, type: 'counter')
                 .info("Finished converting CSV file.")

[rgabo]

Maybe follow in Datadog's footsteps and call it title and text so that it's not tied to the email paradigm? I would consider email as the implementation detail and "title" and "text" the high-level concepts that can be presented muliple ways.

[rgabo]

The project should be built with CircleCI and the README should include a badge from CircleCI. Remove if there is no CI yet.

[martonpe]

What's wrong with Travis? I have already set it up for this project too, but I did forget to update the URL after copying.

[rgabo]

@tmichel do you think it's a good idea to reach into the instance and test against instance variables? That's an implementation detail. I don't see how this test is useful. If the implementation changes, so does the test. As a result, the test is only testing that the syntax is valid, not the functionality.

[tmichel]

I would not test instance variables as well. It might be a good idea to expose a getter method for @event and test against that. If we want to later create different formatters they might need direct access to the event.

As event is already taken we have to resort to get_event or something similar.

[martonpe]

I'm only using instance_variable_get, because I couldn't create a proper attr_reader. The name clashes with the event method. But I'm open for suggestions on how to better test this. My problem with the current method is that this tests both Event and Entry and not just Entry as it should.

[tmichel]

I think it is not a problem here to reach into Event. We that Entry has set up the event correctly based on the inputs.

You cannot use the attr_reader macro here, but you can roll your own reader method:

class Entry
  def get_event
    @event
  end
end

I'm not sure on the name tho.

[rgabo]

class Entry
  def event(name, tags={})
    unless name
      // TODO: Raise error OR just return @event
    end
    ....
  end
end

What do you expect semantically when event is called without arguments? The current implementation is not OK, because it allows calling event without a name.

[tmichel]

It does not allow that. Ruby will throw an error. You could only do entry.event(nil) but not entry.event.

We could make name optional and just return @event in case it is not provided.

[rgabo]

def event(name=nil, tags={})

then?

[tmichel]

Yes we could allow that. Actually we should. As I think more about it, it makes sense that if we call it without any arguments it should just return the current value.

[martonpe]

@rgabo Ready for review again.

• Have you checked with our lawyer yet? We have already released stuff under the MIT license btw.
• Do we want to move off of Travis? Because then Avro2Kafka and CSV2Avro should be moved over to CircleCI as well?

[rgabo]

We can keep the same license as csv2avro and avro2kafka for now.

Yes, we want to get rid of Travis and use a single CI service: CircleCI. Travis costs $125 and the same parallelism would cost $50 at CircleCI. We currently use CircleCI for free.

[rgabo]

There's still a lot of duplication around the different loglevels. Let's do a short refactoring session sometime. Usually it's OK to duplicate in tests if it helps readability, I think we can improve here, but fine for now.

[rgabo]

CircleCI and refactoring can be separate PRs. Great stuff!