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

API Breaking Change in Minor Release: `Route.Handler` -> `Route.Name`? #994

Closed
splittingfield opened this Issue Aug 23, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@splittingfield

splittingfield commented Aug 23, 2017

/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/
/ / / / / / / / / / / / / / / / / /

Please use forum https://forum.labstack.com to ask questions!

/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/
/ / / / / / / / / / / / / / / / / /

Issue Description

In 3.2.2, the Route struct was changed to have a Name method as opposed to Handler.

In 3.2.1,

Route struct {
		Method  string `json:"method"`
		Path    string `json:"path"`
		Handler string `json:"handler"`
	}

In 3.2.2,

	// Route contains a handler and information for matching against requests.
	Route struct {
		Method string `json:"method"`
		Path   string `json:"path"`
		Name   string `json:"name"`
	}

Checklist

  • [ x] Dependencies installed
  • [ x] No typos
  • [ x] Searched existing issues and docs

Expected behaviour

No API breaking changed in minor releases.

Actual behaviour

API breaking changes in a minor release.

Steps to reproduce

Pin Echo to a SemVer range 3.2.x and see API breaking behavior within hours of a new release in which this change was pulled by our build servers.

Version/commit

This change happened in commit
c04b028#diff-252e46e448f4e037d84d69193c8fdf79

@vishr

This comment has been minimized.

Show comment
Hide comment
@vishr

vishr Aug 24, 2017

Member

@splittingfield thanks for reporting, we should have at least made it backward compatible - lesson learned. If you have any suggestion to improve/fix it, please let me know.

Member

vishr commented Aug 24, 2017

@splittingfield thanks for reporting, we should have at least made it backward compatible - lesson learned. If you have any suggestion to improve/fix it, please let me know.

@splittingfield

This comment has been minimized.

Show comment
Hide comment
@splittingfield

splittingfield Aug 24, 2017

@vishr I think this is more of a process thing than anything else. For example, the documentation for Listing Routes is now incorrect. Going forward, (and what I have done in the past) is to have a suite of tests aimed at verifying API compatibility of exposed structs and values. It is so easy to make a change that is completely innocuous, but actually breaks users (such as this one ;-)

Also, documenting changes would help. Is Name the same is Handler? Was Handler removed as a concept? It was unclear to me as to whether we should just change our usage to the new variable or why this change occured.

Also, SemVer is hard ;-)

splittingfield commented Aug 24, 2017

@vishr I think this is more of a process thing than anything else. For example, the documentation for Listing Routes is now incorrect. Going forward, (and what I have done in the past) is to have a suite of tests aimed at verifying API compatibility of exposed structs and values. It is so easy to make a change that is completely innocuous, but actually breaks users (such as this one ;-)

Also, documenting changes would help. Is Name the same is Handler? Was Handler removed as a concept? It was unclear to me as to whether we should just change our usage to the new variable or why this change occured.

Also, SemVer is hard ;-)

@vishr

This comment has been minimized.

Show comment
Hide comment
@vishr

vishr Aug 24, 2017

Member

@splittingfield Here is the complete discussion https://forum.labstack.com/t/named-routes-for-use-in-templates/241

@skyflyer might be able to talk more about it.

I will check on documentation mismatch.

Member

vishr commented Aug 24, 2017

@splittingfield Here is the complete discussion https://forum.labstack.com/t/named-routes-for-use-in-templates/241

@skyflyer might be able to talk more about it.

I will check on documentation mismatch.

@splittingfield

This comment has been minimized.

Show comment
Hide comment
@splittingfield

splittingfield Aug 24, 2017

@vishr Thanks! Echo is awesome by the way. No hard feelings or big deal, just wasted some time to understand what happened.

splittingfield commented Aug 24, 2017

@vishr Thanks! Echo is awesome by the way. No hard feelings or big deal, just wasted some time to understand what happened.

@vishr vishr self-assigned this Aug 24, 2017

@vishr vishr added the docs label Aug 24, 2017

@skyflyer

This comment has been minimized.

Show comment
Hide comment
@skyflyer

skyflyer Aug 25, 2017

Contributor

@splittingfield, that rename was my bad, sorry. And to answer your question:

Name is the same as Handler was before; I wrongly assumed Handler was even used outside of Echo since it is more-or-less an infrastructure thing.

Contributor

skyflyer commented Aug 25, 2017

@splittingfield, that rename was my bad, sorry. And to answer your question:

Name is the same as Handler was before; I wrongly assumed Handler was even used outside of Echo since it is more-or-less an infrastructure thing.

@splittingfield

This comment has been minimized.

Show comment
Hide comment
@splittingfield

splittingfield Aug 25, 2017

No worries my friend. As I said, easy mistake to make and one I am sure I have made myself, but in software no one used so no one noticed 👹

Keep up the great work!

splittingfield commented Aug 25, 2017

No worries my friend. As I said, easy mistake to make and one I am sure I have made myself, but in software no one used so no one noticed 👹

Keep up the great work!

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