Total conversion functions' conventions #74
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carbolymer
force-pushed
the
mgalazyn/adr-use-inject-for-conversion-functions
branch
3 times, most recently
from
8fd1aea to
1585086
Compare
carbolymer
changed the title
Inject draft
carbolymer
changed the title
carbolymer
changed the title
carbolymer
force-pushed
the
mgalazyn/adr-use-inject-for-conversion-functions
branch
4 times, most recently
from
00c20de to
3444bbf
Compare
newhoggy
reviewed
May 21, 2025
newhoggy
reviewed
May 21, 2025
carbolymer
commented
May 21, 2025
newhoggy
reviewed
May 22, 2025
carbolymer
force-pushed
the
mgalazyn/adr-use-inject-for-conversion-functions
branch
2 times, most recently
from
d39cbda to
ea7ef24
Compare
carbolymer
force-pushed
the
mgalazyn/adr-use-inject-for-conversion-functions
branch
2 times, most recently
from
d3d3831 to
756aef0
Compare
Jimbo4350
approved these changes
May 27, 2025
carbolymer
force-pushed
the
mgalazyn/adr-use-inject-for-conversion-functions
branch
2 times, most recently
from
5631f68 to
c9e8e94
Compare
carbolymer
force-pushed
the
mgalazyn/adr-use-inject-for-conversion-functions
branch
from
c9e8e94 to
31dd2a8
Compare
palas
approved these changes
May 27, 2025
|
|
||
| # Context | ||
|
|
||
| In `cardano-api` we have multiple functions performing conversions between one value of the type to the other, for example: |
There was a problem hiding this comment.
Suggested change
| In `cardano-api` we have multiple functions performing conversions between one value of the type to the other, for example: | |
| In `cardano-api` we have multiple functions for performing conversions on values from one type to another, for example: |
|
|
||
| The use of those conversion functions should be limited to **internal use only**. | ||
| The library should still export conversion functions with explicit type names for better readability. | ||
| An exception to this would be a set of types which are convertible one to the other, like `Eon`s. |
There was a problem hiding this comment.
Suggested change
| An exception to this would be a set of types which are convertible one to the other, like `Eon`s. | |
| An exception to this would be a set of types which are all convertible to each other, like `Eon`s. |
I am guessing the all from the N times N below
| The use of those conversion functions should be limited to **internal use only**. | ||
| The library should still export conversion functions with explicit type names for better readability. | ||
| An exception to this would be a set of types which are convertible one to the other, like `Eon`s. | ||
| Writing $N \times N$ conversion functions for $N$ types would be cumbersome and using `inject`/`convert` instead is justified here. |
There was a problem hiding this comment.
Suggested change
| Writing $N \times N$ conversion functions for $N$ types would be cumbersome and using `inject`/`convert` instead is justified here. | |
| Writing $N \times N$ conversion functions for $N$ types would be cumbersome, so using `inject`/`convert` instead is justified here. |
Because it is a consequence, so this would be more precise
| An exception to this would be a set of types which are convertible one to the other, like `Eon`s. | ||
| Writing $N \times N$ conversion functions for $N$ types would be cumbersome and using `inject`/`convert` instead is justified here. | ||
|
|
||
| Inject instances should be placed near one of the types definition, to make them more discoverable and avoid orphaned instances. |
There was a problem hiding this comment.
Suggested change
| Inject instances should be placed near one of the types definition, to make them more discoverable and avoid orphaned instances. | |
| Inject instances should be placed near the definition of one of the types, to make them more discoverable and avoid orphaned instances. |
I think types definition is grammatically incorrect here, it could also be types' definition, for example, but it is less clear
| Inject instances should be placed near one of the types definition, to make them more discoverable and avoid orphaned instances. | ||
|
|
||
| >[!NOTE] | ||
| >The difference between `Inject` and `Convert` class is that `Convert` is better typed for types with `Type -> Type` kind. |
There was a problem hiding this comment.
Nice! I was just asking myself this
|
|
||
| ### Injection law | ||
|
|
||
| The `Inject` and `Convert` classes are meant to be used for trivial conversions only and not for more complex types like polymorphic collections (e.g. `Set a`). |
There was a problem hiding this comment.
What would be the conversion from and to in this example? Set a to what, for example? Or do you mean from one Set to another?
| ```haskell | ||
| import Data.Foo qualified as Foo | ||
|
|
||
| Foo.fromBar bar |
There was a problem hiding this comment.
How about toBar? Are we discouraging that? (Just asking, I don't have an opinion)
| - Less maintenance burden with regards to the naming conventions of the conversion functions | ||
|
|
||
| ## Disadvantages | ||
| - It may be a bit surprising how to discover available conversions, because one would have to browse the type's `Inject` instances to find the conversion functions they are looking for - instead of looking for exported functions. |
There was a problem hiding this comment.
Suggested change
| - It may be a bit surprising how to discover available conversions, because one would have to browse the type's `Inject` instances to find the conversion functions they are looking for - instead of looking for exported functions. | |
| - It may be a bit less obvious how to discover available conversions, because one would have to browse the type's `Inject` instances to find the conversion functions they are looking for - instead of looking for exported functions. |
?
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.