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

Restructure binding guide page #257

Merged
merged 3 commits into from Sep 1, 2022

Conversation

bruc3mackenzi3
Copy link
Contributor

Follow up to this PR, I've made the following changes to Binding guide:

  • Restructure Binding with Struct Tags section into subsections based on included content
    • Collapse the Notes list into subsections accordingly
    • Eliminated some cases where information is repeated
    • Small changes to examples as needed to suit content of their respective subsections
  • Fast Binding Supported Data Types list converted to table

@aldas
Copy link
Contributor

aldas commented Aug 11, 2022

@bruc3mackenzi3 this seems ok but it would be better if we come up better name for that ## Fast Binding with Dedicated Helpers section. I think we should distinquish these 2 different ways of binding by saying that one is something related to struct tags and other is ... this name I do not know - maybe "fluent binder" as the binder API is designed to be fluent. Maybe you have some other suggestions for naming?

and I tagged v4.8.0 yesterday and that release contains 3 new methods for newer binder. See labstack/echo#2127

  • UnixTimeMilli binds 1647184410140 to 2022-03-13T15:13:30.140000000+00:00 useful when dealing with Javascript
  • TextUnmarshaler binds value to destination implementing encoding.TextUnmarshaler interface
  • JSONUnmarshaler binds value to destination implementing json.Unmarshaler interface

@bruc3mackenzi3
Copy link
Contributor Author

@aldas thank you for your review. I will come up with a more suitable name - agreed that Fast Binding with Dedicated Helpers is not great. I must admit I'm not familiar with this part of Echo, so I'll spend some time learning about it.

I will also add these new binding methods to the documentation with your descriptions.

@aldas
Copy link
Contributor

aldas commented Aug 12, 2022

Other languages have "annotation"s as somewhat "similar" to tags. Quick Googling finds threads/blog posts "data annotations vs fluent API" (c#).

Some ideas:
"Struct tags based binding", "Fluent API based binding", "Method/function based binding". "Struct tags vs code base binding" sound too vague.

@bruc3mackenzi3
Copy link
Contributor Author

@aldas I've done some thinking and would like to propose the following renames:

Bind Using Struct Tags -> Struct Tag Binding

Fast Binding with Dedicated Helpers -> Fluent Binding

Custom Binder -> Custom Binding

I like this naming convention for a few reasons: it follows a consistent style, is succinct, and each describes the style of, or approach to, binding.

If we agree on this - or a similar - naming convention, I will also write a brief intro to each section to explain the meaning. E.g. for Fluent Binding:

Echo provides an interface for explicitly binding specific data types from specific sources. It uses method chaining, also known as a Fluent Interface.

@aldas
Copy link
Contributor

aldas commented Aug 19, 2022

@bruc3mackenzi3 this looks definitely better. 👍

@bruc3mackenzi3
Copy link
Contributor Author

@aldas what do you think about removing the supported data types list in favour of a link to the ValueBinder Godoc? I see the following benefits:

  • The Godoc is automatically updated with the code
  • No confusion about which data types are supported for a given version of Echo
  • Unifies the reference from data type to method. As a new user, when I was not familiar with fluent binding it was not obvious to me each data type corresponded to a binding method of the same name

This change might be out of scope for this PR. If you prefer we can discuss elsewhere like in a GitHub issue.

@bruc3mackenzi3
Copy link
Contributor Author

@aldas following up here. I'm not sure if you're on vacation but when you have a chance please take another look.

Are you open to connecting synchronously, say over a video call? I would like to get better aligned with Echo maintainers to facilitate more contributions. Thanks.

@aldas
Copy link
Contributor

aldas commented Aug 26, 2022

sorry, when I am reading Github mails from phone I often forget to reply when I am back at desktop machine (mail being marked as read now in inbox and nothing to remind me about it). If I have not replied in 2 days just tag me again.

I do not know if I am outlier but I find Godocs HTML extremely visually plain/dull, long and I personally rarely read HTML version. Only when I do Google search with specific keyword to see what libraries support some kind of features - even in that case I probably care about function/method name. 99% time I directly hop to code in my IDE and diagonally check Godocs there and scroll through file.
Visually lists do not stand out Godocs and I think there is no way to have a table. Web version Table of Contents / left menu is extremely long (does not fit on one screen) because it list all things from that package - thus being far less useful as a quick overview of capabilities/features.

I see Echo guide/cookbook pages as a condensed, short summaries of Echo features. Godoc gives you everything unfiltered.

We could link/reference to Godoc in binder block but I doubt it would be good substitution for a table/list, for people who just want to scroll diagonally to see if something interesting/useful catches their eyes. Think of people with ADHD :)

p.s. I think cookbook pages would benefit from removing middleware configuration block and linking to that struct in Godoc. For example https://pkg.go.dev/github.com/labstack/echo/v4@v4.7.2/middleware#BasicAuthConfig I find aesthetically very pleasing to watch and It is versioned/up-to-date.

@bruc3mackenzi3
Copy link
Contributor Author

@aldas thank you for clarifying. I will re-ping after a few days if I don't hear from you. How about your typical working hours. I see you're in Estonia, are you working 9-5 Eastern European time, or at other times as well?

I hear what you're saying about Godocs. They are very bland and do not offer much in the way of rich formatting.

I will add the new binder methods shortly. Please let me know if you'd like to see any other changes, I'd like to wrap this up and move on to another rewrite :)

@aldas
Copy link
Contributor

aldas commented Aug 26, 2022

I am available most of the time 8am-24pm (UTC+3) including weekends - basically when I am at my PC. Most of the Echo stuff gets replied in less than 24h. I additionally check Echo repo issues/discussions page quite often but echox and echo-contrib stuff gets forgotten time-to-time. I do not visit these repos that much. The volume of issues is significantly smaller in these repos compared to Echo main repo.

@bruc3mackenzi3
Copy link
Contributor Author

@aldas apologies for the delay at my end. I've added the three binding methods recently released.

Please give another review. I'd like to wrap this up and get it merged. Re. the suggested cookbook change, I'd prefer to tackle it in another PR.

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

LGTM

@aldas aldas merged commit 7b44456 into labstack:master Sep 1, 2022
@aldas
Copy link
Contributor

aldas commented Sep 1, 2022

Thank you @bruc3mackenzi3

@bruc3mackenzi3
Copy link
Contributor Author

@aldas re. your comment:

cookbook pages would benefit from removing middleware configuration block and linking to that struct in Godoc.

Can you link to the exact place in the cookbooks you're referencing. I took a look and couldn't find it.

@aldas
Copy link
Contributor

aldas commented Sep 1, 2022

sorry, I meant middleware pages https://echo.labstack.com/middleware/basic-auth/#configuration we often forget to update these blocks

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

Successfully merging this pull request may close these issues.

None yet

2 participants