HTTP::FixedLengthContent not documented #9444

Daniel-Worrall · 2020-06-09T07:44:29Z

For HTTP::Client streaming, body_io is an HTTP::FixedLengthContent and intended to be interacted with but this is no-doc'd.

From the docs

require "http/client"

HTTP::Client.get("http://www.example.com") do |response|
  response.status_code  # => 200
  response.body_io.gets # => "<!doctype html>"
end

In my use-case, I needed response.body_io.as(HTTP::FixedLengthContent).read_remaining

The text was updated successfully, but these errors were encountered:

jhass · 2020-06-09T08:40:57Z

Mmh, read_remaining is just IO::Sized#read_remaining, which is public.

In general either should be transparent and treated like a plain IO. Can you expand a bit on your usecase that requires you to access read_remaining?

Daniel-Worrall · 2020-06-09T10:13:06Z

My use-case is detailed further in #9445 where I wish to get the progress of a large downloaded file

Daniel-Worrall · 2020-06-10T20:43:23Z

I overlooked that this was coming from IO::Sized, so will close

straight-shoota · 2020-06-10T23:19:33Z

There's however a problematic aspect here: Stdlib exposes undocumented types in its API. When a returned value is of type HTTP::FixedLengthContent a user would expect to find documentation about that. Even the fact that it inherits from IO::Sized is far from obvious.

Even if the implementation is only an internal detail, I think everything that is exposed through the public API should be documented.

asterite · 2020-06-11T00:21:21Z

You mean that the runtime is returning a type that the user can see, but it's not documented, so it should be documented? That's not the right thing to do. The right thing to do is for the user to see that this type is not documented and so they shouldn't fiddle with it.

bararchy · 2020-06-11T06:42:29Z

@asterite I've gotten this type when using HTTP::Server and handling request-response, this should be documented as you actually do interact with it.
response.body.to_s is usually expected to return the body, but when the body is HTTP::FixedLengthContent you just get the class name as a string, and this is indeed confusing.

jhass · 2020-06-11T07:06:34Z

So we got this fancy return type annotations y'all like so much. What happens if we add one to IO::Sized here?

asterite · 2020-06-11T09:08:37Z

If you introspect code in Java, C#, etc., you can get named of private types. That doesn't mean they should be documented... this has been like that for years already. It's just nonsense to start documenting internal types.

straight-shoota · 2020-06-11T09:46:14Z

The right thing to do is for the user to see that this type is not documented and so they shouldn't fiddle with it.

But when such a type is returned, it's for the purpose of using that type, right? How's the user supposed to know how to do that when it's not documented? It's just useless without any information.

In Java, this is not so much a problem because type restrictions work differently and apply stronger. The stdlib typically uses public interfaces as fronts for internal types. So while the actual type might be some internal private class, to the user it behaves as the interface type. You can't call methods on the implementation type if they're not in the inteface. The internal type is only visible when you ask for it (for example calling getClass() or toString()) and its only usable if you explicitly cast to that type. Otherwise, it's just treated as the interface type everywhere, in code and documentation.

That's all different in Crystal. Even if you use an interface as type restriction, the compiler still resolves that to the actual types covered by that (more or less) and to the user there's no difference from using the private type.

Example:

interface Foo { }
private class FooImpl implements Foo {
  public void implementationDetail() { }
}

Foo getAFoo() {
  return new FooImpl();
}

// This works:
Foo foo = getAFoo();
// This doesnt:
FooImpl foo = getAFoo();
// This doesn't work in either case:
foo.implementationDetail();

module Foo; end
class FooImpl
  include Foo
  def implementation_detail
  end
end
 
def get_a_foo : Foo
  FooImpl.new
end

# Both work:
foo : Foo = get_a_foo()
foo : FooImpl = get_a_foo()
# This also works:
foo.implementation_detail()

asterite · 2020-06-11T11:23:36Z

What method are we talking about? If it's body_io, that's an IO.

straight-shoota · 2020-06-11T12:14:33Z

The type restriction of Response#body_is is IO, but it still returns a FixedLengthContent and allows calling methods on that type. I just pointed out that that's different in Java where the interface return type shadows all more specific type information.

asterite · 2020-06-11T12:26:13Z

@straight-shoota Wrong, because when you execute the code with a Java debugger you will see the actual type.

asterite · 2020-06-11T12:26:47Z

What people are proposing here basically means "let's get rid of private types and hiding implementation details".

straight-shoota · 2020-06-11T12:41:35Z

Yes, in Java you can still reflect on the value and determine the actual type. I've mentioned this explicitly in my previous comment.
I'm talking about the type grammar. In the Java example above, the return type of getAFoo() is just Foo. There's no way to trace that it's a FooImpl without looking at an actual instance value at runtime.

In Crystal however, even though get_a_foo is restricted to Foo, the compiler knows that it returns FooImpl at compile time and that's how it's typed: typeof(get_a_foo) # => FooImpl.

So other than Java, users of such a method in Crystal easily get to interact with the supposedly private, internal type, thus leaking that implementation detail to the public.

asterite · 2020-06-11T12:43:48Z

You know what other language works that way? Ruby! Yes, Ruby leaks implementation everywhere. Ruby was and is still our inspiration. I never found this to be a problem in Ruby. If there's a type I don't know and it's not documented, I don't fiddle with it. Or yes, I go to the source code and check how it works, but I never try to use their methods because I know they are implementation details.

asterite · 2020-06-11T12:45:40Z

This is probably true also in Python and many other dynamic languages. Yes, at runtime, or even at compile-time in Crystal you will learn the types with typeof. That's never been a problem so far and I don't think we should change anything of this.

If we really want to change it we need to automatically cast return values to their return type annotations. Maybe that's a solution, I don't know... I really don't think this is an issue.

straight-shoota · 2020-06-11T12:52:57Z

For languages like Ruby and Python that's not so much of an issue. In dynamically typed languages developers tend to care less about types. It's pretty rare that you do any type checks in those languages. And it's all happening at runtime, depending on the actual values, not type grammar.

I honestly don't know what a good solution would be. I just notice that the current situation is not ideal because private details are leaking into the public. Because of the way Crystal's type grammar works, that's has a stronger effect than in Java, Ruby or Python.

asterite · 2020-06-11T12:56:54Z

Yeah... maybe we can try casting return values to the return type annotations. But that menas that if a method returns HTTP::FixedLengthContent but it says it returns IO, the effective type will be IO, which is represented as possibly all IOs in the program, so doing write or any other method will do a dispatch, which results in more code being generated, and also slower code. So there's that trade-off.

Also sorry if any of what I said above (or in other issues) sounded a bit harsh. My kid has troubles falling asleep and so am I. I guess it affects my mood whether I want it or not.

jhass · 2020-06-11T13:09:07Z

Maybe a notion of a type being different for semantic and codegen, with the constraint that the codegen type must be a subset of the semantic type, could be useful, in general? Here that would allow us to have the semantic type be that of the return type restriction, while preserving the actual concrete type for codegen.

But yeah, that's a bigger internal change.

straight-shoota · 2021-11-24T19:22:23Z

This has come up again in chat: https://discord.com/channels/591460182777790474/591597160492171264/913141422218149929
HTTP::Request#body returns an instance of HTTP::FixedLengthContent and when people try to look that up in the API docs, they find nothing.
That's really bad developer experience.

Undocumented internal types that leak through the public API are usually (or hopefully always) implementations of specific interfaces. The interfaces themselves are documented and the rationale is that the implementation does not need any documentation because the type itself is just a negligible internal detail and its API is completely generic regarding the interface.

I think one thing we could improve is to change #inspect on internal types. I wouldn't completely hide the class name, but it should at least mention which interface it implements, so people can look that up instead of the class name which yields nothing.

So maybe instead of

#<HTTP::FixedLengthContent:0x7f5b8b1f0bd0 ...>

we could have something like

#<IO::Sized as HTTP::FixedLengthContent:0x7f5b8b1f0bd0 ...>

jly36963 · 2022-08-18T16:49:07Z

I'm experiencing this exact scenario. I'm trying to use env.request.body in the kemal framework, which returns type HTTP::FixedLengthContent, and has no public documentation.

Daniel-Worrall closed this as completed Jun 10, 2020

straight-shoota reopened this Nov 24, 2021

straight-shoota added status:discussion topic:stdlib kind:docs labels Nov 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP::FixedLengthContent not documented #9444

HTTP::FixedLengthContent not documented #9444

Daniel-Worrall commented Jun 9, 2020

jhass commented Jun 9, 2020

Daniel-Worrall commented Jun 9, 2020

Daniel-Worrall commented Jun 10, 2020

straight-shoota commented Jun 10, 2020

asterite commented Jun 11, 2020

bararchy commented Jun 11, 2020

jhass commented Jun 11, 2020

asterite commented Jun 11, 2020

straight-shoota commented Jun 11, 2020

asterite commented Jun 11, 2020 •

edited

straight-shoota commented Jun 11, 2020

asterite commented Jun 11, 2020

asterite commented Jun 11, 2020

straight-shoota commented Jun 11, 2020

asterite commented Jun 11, 2020 •

edited

asterite commented Jun 11, 2020

straight-shoota commented Jun 11, 2020

asterite commented Jun 11, 2020

jhass commented Jun 11, 2020 •

edited

straight-shoota commented Nov 24, 2021

jly36963 commented Aug 18, 2022

HTTP::FixedLengthContent not documented #9444

HTTP::FixedLengthContent not documented #9444

Comments

Daniel-Worrall commented Jun 9, 2020

jhass commented Jun 9, 2020

Daniel-Worrall commented Jun 9, 2020

Daniel-Worrall commented Jun 10, 2020

straight-shoota commented Jun 10, 2020

asterite commented Jun 11, 2020

bararchy commented Jun 11, 2020

jhass commented Jun 11, 2020

asterite commented Jun 11, 2020

straight-shoota commented Jun 11, 2020

asterite commented Jun 11, 2020 • edited

straight-shoota commented Jun 11, 2020

asterite commented Jun 11, 2020

asterite commented Jun 11, 2020

straight-shoota commented Jun 11, 2020

asterite commented Jun 11, 2020 • edited

asterite commented Jun 11, 2020

straight-shoota commented Jun 11, 2020

asterite commented Jun 11, 2020

jhass commented Jun 11, 2020 • edited

straight-shoota commented Nov 24, 2021

jly36963 commented Aug 18, 2022

asterite commented Jun 11, 2020 •

edited

asterite commented Jun 11, 2020 •

edited

jhass commented Jun 11, 2020 •

edited