Skip to content
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

Output formats (json) for lavinmqctl #790

Merged
merged 14 commits into from
Oct 7, 2024

Conversation

viktorerlingsson
Copy link
Member

WHAT is this pull request doing?

Adds an option to specify output format (json) for lavinmqctl cmds. Also changes all methods to use the same output method.

Example usage:

$ bin/lavinmqctl list_queues -f json
[{"name":"amq.delayed.delay-test","messages":"0"},{"name":"perf-test","messages":"0"},{"name":"delay-q-test","messages":"0"},{"name":"delay2","messages":"0"},{"name":"q1","messages":"0"},{"name":"q2","messages":"0"},{"name":"federation: ex1 -> viktor-lenovo:/:ex1","messages":"0"}]

HOW can this pull request be tested?

Manual

src/lavinmqctl.cr Outdated Show resolved Hide resolved
src/lavinmqctl.cr Outdated Show resolved Hide resolved
src/lavinmqctl.cr Show resolved Hide resolved
src/lavinmqctl.cr Outdated Show resolved Hide resolved
src/lavinmqctl.cr Outdated Show resolved Hide resolved
src/lavinmqctl.cr Outdated Show resolved Hide resolved
src/lavinmqctl.cr Outdated Show resolved Hide resolved
else
if first = data.first?
first = first.as_h if first.is_a?(JSON::Any)
puts first.keys.join("\t")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
puts first.keys.join("\t")
puts first.each_key.join(STDOUT, "\t")

Copy link
Member Author

Choose a reason for hiding this comment

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

each_key is expected to be invoked with a block, so this throws an error.
Maybe just puts first.keys.join(STDOUT, "\t") instead?

Copy link
Member

Choose a reason for hiding this comment

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

What types may data contain? Hash#each_key returns an iterator (if not invoked with a block), but NamedTuple#each_key only has one overload that requires a block.

Copy link
Member Author

Choose a reason for hiding this comment

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

data.first will either be a NamedTuple or a JSON::Any

Copy link
Member Author

@viktorerlingsson viktorerlingsson Sep 27, 2024

Choose a reason for hiding this comment

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

so maybe something like this?

        if first.is_a?(NamedTuple)
          puts first.keys.join(STDOUT, "\t")
        elsif first.is_a?(JSON::Any)
          puts first.as_h.each_key.join(STDOUT, "\t")
        end

Copy link
Member

Choose a reason for hiding this comment

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

Yes. But IMO a case statement could is more readable than if statement when doing type checks.

data.each do |item|
values = [] of String
if item.is_a?(Hash) || item.is_a?(NamedTuple)
values = item.values.map &.to_s
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
values = item.values.map &.to_s
puts item.each_value.join(STDOUT, "\t")

Copy link
Member Author

Choose a reason for hiding this comment

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

each_value expects to be invoked with a block, so this throws an error. Maybe just print each value instead of the join?

    data.each do |item|
      if item.is_a?(Hash) || item.is_a?(NamedTuple)
        item.each_value { |v| print "#{v}\t" }
      elsif item.is_a?(JSON::Any)
        if hash = item.as_h
          hash.each_value { |v| print "#{v}\t" }
        end
      else
        print item.to_s
      end
      puts
    end

Copy link
Member

@spuun spuun Sep 27, 2024

Choose a reason for hiding this comment

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

You can do like this:

data.each do |item|
  case item
  when Hash
    item.each_value.join(STDOUT, "\t")
  when JSON::Any
    item.as_h.each_value.join(STDOUT, "\t")
  when NamedTuple
    item.keys.each.join(STDOUT, "\t")
  else
    item.to_s(STDOUT)
  end
  puts
end

NamedTuple#keys returns a tuple, so no allocation.

Copy link
Member Author

Choose a reason for hiding this comment

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

nice 👍 (but we want the values from the NamedTuple so we should use NamedTuple#values instead I guess. Also returns a tuple)

Copy link
Member

@spuun spuun Sep 27, 2024

Choose a reason for hiding this comment

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

Oh, right! 😁
It was in the previous thread it was keys that was mentioned.

Copy link
Member

Choose a reason for hiding this comment

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

Btw, maybe you can change item.keys.each.join(STDOUT, "\t") to just item.keys.join(STDOUT, "\t")?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes 👍

src/lavinmqctl.cr Outdated Show resolved Hide resolved
src/lavinmqctl.cr Outdated Show resolved Hide resolved
src/lavinmqctl.cr Outdated Show resolved Hide resolved
src/lavinmqctl.cr Outdated Show resolved Hide resolved
src/lavinmqctl.cr Outdated Show resolved Hide resolved
src/lavinmqctl.cr Outdated Show resolved Hide resolved
viktorerlingsson and others added 5 commits September 26, 2024 14:11
Co-authored-by: Carl Hörberg <[email protected]>
Co-authored-by: Carl Hörberg <[email protected]>
Co-authored-by: Carl Hörberg <[email protected]>
Co-authored-by: Carl Hörberg <[email protected]>
@viktorerlingsson viktorerlingsson merged commit 54d1c84 into main Oct 7, 2024
24 of 25 checks passed
@viktorerlingsson viktorerlingsson deleted the lavinmqctl_output_formats branch October 7, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants