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

Implement power supply and disk #20

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

andraantariksa
Copy link

PR for #19

Copy link
Collaborator

@j8r j8r left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @andraantariksa, very appreciated! I left several comments.

# # `ls /sys/class/power_supply/`
# power_supply = Hardware::PowerSupply.new("BAT0") # Specify the batery device name by yourself
# ```
class Hardware::PowerSupply
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use abstract struct because:

  • this objects are immutable
  • we usually want to use Hardware::Battery instead of Hardware::PowerSupply if we have a battery.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, a dedicated error type can be created inside like:

class Error < Exception
end

Copy link
Author

Choose a reason for hiding this comment

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

I would use abstract struct because:

  • this objects are immutable
  • we usually want to use Hardware::Battery instead of Hardware::PowerSupply if we have a battery.

I'm using class because it allows me to use inheritance and to define Hardware::PowerSupply as non abstract so it may be used

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know why one would use PowerSupply directly, because it is either a UPS, Battery, Main or USB.

Copy link
Author

Choose a reason for hiding this comment

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

After I think it again, PowerSupply is not necessary to be used directly. I have made it into an abstract struct..

src/power_supply.cr Outdated Show resolved Hide resolved
src/power_supply.cr Outdated Show resolved Hide resolved
src/power_supply.cr Outdated Show resolved Hide resolved
src/power_supply.cr Outdated Show resolved Hide resolved
src/power_supply/battery.cr Show resolved Hide resolved
src/power_supply/battery.cr Outdated Show resolved Hide resolved
src/power_supply.cr Outdated Show resolved Hide resolved
spec/power_supply_spec.cr Show resolved Hide resolved
src/power_supply/battery.cr Show resolved Hide resolved
src/power_supply.cr Show resolved Hide resolved

# Select one of the battery device available
def initialize
battery_power_device = PowerSupply.entries.find { |entry| entry.is_a? Battery }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, using an intermediate arrray is not efficient: use PowerSupply.each, and break if an entry match Battery

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you resolve some of this un-resolved comments like this one?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, that's because I have solve it locally 😅. I have changed this into

def self.new : Hardware::Battery
    battery_power_device : Battery | Nil = nil
    PowerSupply.each.each do |device_name|
      temp_device : PowerSupply = PowerSupply.new_with_type(device_name)
      if temp_device.is_a?(Battery)
        battery_power_device = temp_device
        break
      end
    end
    if battery_power_device
      battery_power_device
    else
      raise InvalidDevice.new "No battery device found"
    end
  end

Where PowerSupply.each is

def self.each : Iterator(String)
  Dir.new(device_directory).each_child
end
``

end

# Reports the serial number of the device
def serial_number : String | Nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

This methods must return String. Use getter model_name : String do ... end, it is a convenient wrapper for memoization.

(Same with other methods below)

Copy link
Author

Choose a reason for hiding this comment

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

Okay, so it will be something like this right?

@serial_number : String | Nil = nil
# Reports the serial number of the device
getter serial_number : String do
  unless @serial_number
    @serial_number = File.read(@current_device_path / "serial_number").rstrip
  end
  @serial_number
end

end

# Select one of the battery device available
def initialize
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use self.new, and return the found, already allocated Battery.

src/power_supply.cr Outdated Show resolved Hide resolved
src/power_supply.cr Outdated Show resolved Hide resolved
@j8r
Copy link
Collaborator

j8r commented Jul 26, 2020

If you don't see what I mean in the comments, I can push a commit which implements my proposals (after you are done of course). Then, you can review it too.

@j8r
Copy link
Collaborator

j8r commented Jul 26, 2020

I propose to merge this when the test are executed (somehow) in the CI. Then, I can open a PR to do minor improvements here and there.

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.

2 participants