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

Complete all methods #6

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

Complete all methods #6

wants to merge 3 commits into from

Conversation

dukenguyenxyz
Copy link
Contributor

@dukenguyenxyz dukenguyenxyz commented Mar 8, 2021

  • Use enums instead of strings
  • Complete specs

@dukenguyenxyz dukenguyenxyz self-assigned this Mar 8, 2021
struct PutResponse < Base
getter header : Header
struct PutResponse < WithHeader
# Why is this nillable
getter prev_kv : Kv?
Copy link
Contributor

Choose a reason for hiding this comment

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

Nillable because the previous is only included if you ask for the previous kv.

Copy link
Contributor

Choose a reason for hiding this comment

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

And it might not be present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Theres no nillable type marking on the openapi specs, makes everything a bit more confusing than it should be

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should work against the API rather than the spec. I remember it not being consistent.

src/etcd/model/lease.cr Outdated Show resolved Hide resolved
src/etcd/model/lease.cr Outdated Show resolved Hide resolved
src/etcd/model/maintenance.cr Outdated Show resolved Hide resolved
src/etcd/model/maintenance.cr Outdated Show resolved Hide resolved
src/etcd/model/base.cr Outdated Show resolved Hide resolved
src/etcd/model/auth.cr Outdated Show resolved Hide resolved
src/etcd/model/auth.cr Outdated Show resolved Hide resolved
src/etcd/model/auth.cr Outdated Show resolved Hide resolved
@dukenguyenxyz dukenguyenxyz changed the title DRAFT: Complete all methods Complete all methods Mar 8, 2021
@dukenguyenxyz dukenguyenxyz requested a review from caspiano March 8, 2021 05:52
src/etcd/auth.cr Outdated
end

private def validate!(name : String)
raise ArgumentError.new("Arg name is empty") if name.empty?
Copy link
Contributor

@caspiano caspiano Mar 8, 2021

Choose a reason for hiding this comment

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

You can put an always inline annotation above this. That will inline the code (i.e. just drop the method inline in the method, reducing the overhead of a method call (i.e. setting up a stack)

The message is also a little vague. Something closer to the message below would be preferrable.

Argument `name` is empty.

src/etcd/cluster.cr Show resolved Hide resolved
src/etcd/cluster.cr Outdated Show resolved Hide resolved
src/etcd/model/kv.cr Outdated Show resolved Hide resolved
src/etcd/lease.cr Outdated Show resolved Hide resolved
@caspiano caspiano added the type: enhancement new feature or request label Jul 7, 2021
@caspiano caspiano force-pushed the feat/update branch 2 times, most recently from bd873e2 to b43427e Compare November 15, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants