Skip to content

Conversation

HertzDevil
Copy link
Contributor

No description provided.

@straight-shoota
Copy link
Member

I'm wondering about the motivation for this. I suppose it's to encapsulate all URI-related stuff behind require "uri"? I suppose that makes some sense, but is there anything wrong with the current way? There is surely some merit to having the method sit with the target type instead of the return type.

I'm not opposed, just want to think things through, so we leave a paper trail explaining the motivation instead of merging this change without a comment.

A practical concern is the error behaviour:
Currently, when calling Path#to_uri without require "uri", you'll get a compiler error undefined constant URI. With this patch, the error message is undefined method 'to_uri' for Path. IMO the former is a bit more helpful because it points out that URI is missing, and looking at the API docs it says that you need to explicitly require it (this patch also adds that to the method docs, so maybe it's not a big concern?).

There could also be a general discussion about code organization. We probably wouldn't want to merge all #to_slice methods in a single place (to_slice.cr). But we do that for #to_json and #to_yaml for example, because those are methods that should only be available after requiring JSON, or YAML.
The return types of most #to_* types are always available in core lib, so there's less incentive to move them away behind a require. I suppose it can generally make sense for return types that are not in core lib. But I'm not sure if this should be a "must" for all cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants