Using records for more complicated functions

[kornicameister]

The idea to enhance is to change the signature of functions that have a lot of arguments with a verbose record.

That would be more informative IMHO compared to normal arguments.

I can drop a PR if you like.

[Lattyware]

This sounds like a really good idea. The lib started with very simple functions and slowly grew, but you are right it is well past the point where a record would be far better.

I'd absolutely welcome a PR for this change.

As a note, if you do make this change, I'd say keep around the old function signatures and just point them at the new ones - this avoids breaking API compatibility. In the docs, just shove them at the bottom and mark them as deprecated.

[kornicameister]

Keep'em in the docs you mean?

[Lattyware]

The elm package tool will reject the package if there are undocumented public functions, so yeah.

Likewise, if functions are removed, it'll require a major version bump and people would need to refactor to use the new version. Providing the old interfaces as deprecated means people can update without issue, but new users should start using the new interface. Putting the new functions at the top should help new users pick the best ones.

[kornicameister]

Ugh :( ...I know how Elm versioning system works but only in details. Never had pushed a library or anything, so I will need some assistance :). I will try to sit down to this once I clear some other assignments.

Cheers!

[Lattyware]

No problem - I'll need to be the one to actually push the package up anyway as I own the repo. It'll be trivial changes needed if any, so don't sweat too much about it. Feel free to ping me here if you need any assistance - or just push up the PR and leave comments on that if you want to point at specific code.

[kornicameister]

I have been giving this matter a little bit of thinking recently and honestly speaking and frankly I stopped seeing a point. I though it might be good to introduce these records but then again I realized that the function I have been using the most was viewStyled which signature is not too much complicated.

That being said, sorry given how those functions are organized, i.e. to mimic the Html members having attributes always in the first place, I do not see if there is any way to improve current situation.

I mean, I've tried different ways to express this but all of my attempts made API look odd, to say the least.

Not sure what we can do from here :(

[Lattyware]

I took a look at this myself, and I've just pushed (just here 0c77e65ce9942ead2903c0b6b30af11be1385f16, no new elm package yet) a version with a pipeline API based on records. I'd be interested in hearing your thoughts on it.

The main pain point is the need for both view and viewId methods, but I can't see a way around that (switching to using a union type would solve the issue, but moves it to the titled and masked methods, which then can't only work on one side).

[kornicameister]

I don't want to be rude, but the API you are proposing does not seem to solve the issue. I'd even say that it makes API a little bit hard to follow. Take a look here. You're saying that by Identified means an Icon that has id, title and for some reason outer icon. It's just seems to much to comply to Identified which, IMHO, should be just id.

That being said, and without any hard feeling, if I were to judge that code, I'd say "no" :(.

However, there was one other thing that gave me some thinking from the link you gave. What if we use this pipeling and make functions so that following is possible:

Icon Icon.car
  |> Icon.withId "myCardId"
  |> Icon.withTransform carTransformProps
  |> Icon.withTitle "My car"
  |> Icon.withStyle [Icon.fw, Icon.fa2x]
  |> Icon.toHtml

that would sort of mean that anything else other than just pure icon would go into optional fields of a record like so:

type Icon = 
   Icon IconProperties

type alias IconProperties = {id: Maybe String, title: Maybe String, transform: List Transform, ...}

I don't know how to solve the idea that stands behind viewMasked or in general stacking font awasome icons, but I hope you get my point.

I am proposing something radical as that, because it seems to be that if a problem is a lot of arguments which are in fact not necessary in most of the cases we can conclude that those arguments are in fact optional. Obviously speaking **drawback*** is that an API is not backward compatible but the benefits are that you are free to manipulate icon rendering process based on single record that user can only modify in controlled way via functions like Icon.withTitle. If user wishes to add id he can choose to use Icon.withId but he's not forced to do so.

An alternative is to expose Html like API only for simple icons and a function Icon.build or Icon.pipe that would allow to compose an Icon freely as it is needed.

I hope that this helps instead and not dissapoints.

[Lattyware]

Your example is essentially how the API I implemented is used. I probably should have shown an example. The details are a little different, but it ends up as:

Icon.present Icon.car 
  |> Icon.withId "car"
  |> Icon.transform [...]
  |> Icon.titled "My car"
  |> Icon.styled [...]
  |> Icon.viewId

It is intentional that the interface with the Identified type contains the outer icon and title - both of these require references to the id in the resulting HTML, meaning that if you don't specify a unique id, but do specify one of the other components, we would have to choose a default, and if used twice you will produce invalid HTML (the same id on two elements).

The old API just described this problem in the documentation, but this API ensures you have to manage the ids, making it harder to just assume the default can be repeated. The JS library generates random ids, which we can't do in Elm without a lot of visibility in the API due to the lack of side effects. Exposing it this way lets the user manage the uniqueness of IDs - either by just manually managing them or generating them somehow (including randomly).

Not being backwards compatible isn't a problem, as in my implementation, we can simply implement this on top of the old API.

[Lattyware]

An altered version of this that has a slightly nicer interface (only one view function) is in as of 3.0.0.