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

More granular v2 to v3 upgrade documentation? #2989

Open
stevenmaguire opened this issue Apr 1, 2024 · 3 comments
Open

More granular v2 to v3 upgrade documentation? #2989

stevenmaguire opened this issue Apr 1, 2024 · 3 comments
Labels
documentation Rely to documentations or gh-pages branch

Comments

@stevenmaguire
Copy link

I have read the upgrade guide on the documentation site: https://carbon.nesbot.com/docs/#api-carbon-3

I've worked through the upgrade guide and I am still catching errant issues related to changes in parameter types of methods here and there. For instance createFromFormat (which invokes rawCreateFromFormat with similar signature) was previously untyped and now explicitly expects, for example, a string for parameter # 2, $time.

Example: A valid use case currently supported by v2 is passing in a year and month (202401), which happens to be an int, in conjunction with a `Ym' format.

This issue is not intended to open a discussion about the choices for what types are expected, but rather a consideration for additional documentation to explicitly clear up the public methods which need attention during a proactive upgrade effort. A punch list of sorts?

The diffIn* section is forthcoming about significant issues related to the return type changes and offers the most detailed guidance.

I am left worried that the inventory of 400+ public methods contains more undocumented breaking changes than currently hinted at in the ~500 word guide at the link above.

Is there bandwidth to update the upgrade guide with a more detailed inventory of which of those 400+ public methods contain, not just return type changes, but also parameter type changes? With acknowledgement that going from no type to a type is a type change.

@stevenmaguire
Copy link
Author

In case anyone else is interested in this problem...

I built a little utility to use reflection to analyze v2.x and v3.x of this package. The comparison surfaces:

  • 241 public methods where Return Type, one or more Parameter Type(s), or both have changed
  • 4 public methods where Return Type and Parameter Type did not change, but the parameter variable name did change
  • 25 removed public methods
  • 6 added public methods
  • 104 unchanged public methods

The 241 (minus the 20 diffIn* methods that are detailed in the upgrade guide) method changes where return type and/or parameter types were changed are likely going to be the biggest source of heartburn for an upgrade effort.

@kylekatarnls kylekatarnls added the documentation Rely to documentations or gh-pages branch label Apr 2, 2024
@kylekatarnls
Copy link
Collaborator

As for the library code, the documentation is open-source and contributions are more than welcome:
https://github.com/briannesbitt/Carbon/blob/gh-pages/docs/index.src.html#L5033

But I also think that expanding the migration guide for every single change can also make it tedious for most people who may have a limited usage of Carbon and for which the current guide might cover all they need to upgrade.

So it could be something with folded sections summarizing of a category of methods with some "Read more" button.

FWIW, type changes should more closely follow semantic now. For instance createFromFormat('Y', 2024) make some sense as year is a single integer value. But createFromFormat('Ym', 202401) is actually not semantically correct, it's using decimal "trick" but Ym expects concatenation of year and month with trailing zero as strings, not year * 100 + month as integer.

@stevenmaguire
Copy link
Author

@kylekatarnls thanks for the response and acknowledgement.

First, it is not my intention here to throw this onto you and ask you to fix it alone. I am willing to help - if there was consensus around the direction.

This is a large library and it is impossible to ignore that since it is baked right into Laravel that a lot of people depend on it AND it likely has a widespread usage in many deployments - from views, models, actions, queue jobs, commands, etc. It is unlikely to expect that someone can just do a simple find and replace before dusting their hands off and calling it done. I would also venture a guess to say that most developers are not wrapping every Carbon method in a try/catch, so exceptions being thrown in unexpected places will be a problem unless someone has 100% automated functional test coverage.

So, the main direction of my ask is to try to provide some guidance on proactively narrowing the scope.

I appreciate that the docs are open source and I am happy to contribute. Considering you have feelings about the scope and size of documentation I would like a little perspective on what kind of changes you think would be appropriate. For instance, I am happy to share the analysis I generated. It is a CSV with all of the methods and changes. Perhaps a note about the size of the changes due to a shift to more strongly typed method signatures and a link to download the CSV?

What kind of appetite do you have for perhaps triggering E_USER_DEPRECATED warnings in v2.x on all of the methods where the parameter and/or return types are expected to change? This would at least give a consuming application a chance to subscribe to and inventory the methods that are actually being consumed, instead of going through all 241 methods from the aforementioned report only to discover that 10% are of concern?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Rely to documentations or gh-pages branch
Projects
None yet
Development

No branches or pull requests

2 participants