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

start upgrade guide, and add pieces from LegolasFlux upgrade #71

Merged
merged 6 commits into from
Nov 22, 2022

Conversation

ericphanson
Copy link
Member

@ericphanson ericphanson commented Nov 22, 2022

when @kleinschmidt and I upgraded LegolasFlux to Legolas v0.5, we encountered several points that we thought would be useful to as upgrade guidance for our users. All of those points seem general and not specific to LegolasFlux however, so I think it would be more appropriate to put it in Legolas's documentation itself.

This adds incomplete documentation (since I don't think this is a full upgrade guide), but it is deliberately called out as such. IMO it would be better to incrementally add pieces as folks find things rather than block such guidance on being complete.

ref #57

docs/src/upgrade.md Outdated Show resolved Hide resolved
ericphanson added a commit to beacon-biosignals/LegolasFlux.jl that referenced this pull request Nov 22, 2022
requires beacon-biosignals/Legolas.jl#71; currently points to preview docs, but we should land that, then update the link here to point to dev or stable docs.

xref #21
Copy link
Member

@kleinschmidt kleinschmidt left a comment

Choose a reason for hiding this comment

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

looks good with one little quibble! I agree this is a good place for it to live

docs/src/upgrade.md Outdated Show resolved Hide resolved
@hannahilea
Copy link
Contributor

:glue: ❤️ :teamwork-makes-the-dreamwork:

Co-authored-by: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
docs/src/upgrade.md Outdated Show resolved Hide resolved
Co-authored-by: Jarrett Revels <jarrettrevels@gmail.com>
docs/src/upgrade.md Outdated Show resolved Hide resolved
docs/src/upgrade.md Outdated Show resolved Hide resolved
Copy link
Member

@jrevels jrevels left a comment

Choose a reason for hiding this comment

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

LGTM pending folding in suggestions!

Co-authored-by: Jarrett Revels <jarrettrevels@gmail.com>
Copy link
Member

@jrevels jrevels left a comment

Choose a reason for hiding this comment

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

not sure i understand the "an example of a comprehensive log of changes" change (it's not an example of a changelog, it's just the changelog that accompanies the PR) but doesn't matter either way lol

docs/src/upgrade.md Outdated Show resolved Hide resolved
@ericphanson
Copy link
Member Author

not sure i understand the "an example of a comprehensive log of changes" change (it's not an example of a changelog, it's just the changelog that accompanies the PR) but doesn't matter either way lol

Oh oops, somehow I thought that link pointed at beacon-biosignals/LegolasFlux.jl#21 as an upgrade example, in which case it wouldn't be a comprehensive log of changes to this package. But I see the link points to #54, which makes more sense, and the wording made sense as-is.

@jrevels
Copy link
Member

jrevels commented Nov 22, 2022

just realized it might be mildly useful for me to provide slightly more context on my thought process for one of the suggested changes we folded in (and to surface for lurkers who might've missed the nested suggestion in the first place)

- Constructors for structs generated by `@version` calls discard "extra" columns, 
- whereas previously under Legolas v0.4, `@row`-generated constructors kept any "extra" 
- columns. It may be advisable to add previously used "extra" columns into the schema itself 
- (probably declared as `::Union{Missing,T}` to allow them to be missing).
+ In Legolas v0.4,  `@row`-generated `Legolas.Row` constructors accepted and propagated any 
+ non-schema-required fields provided by the caller. In Legolas v0.5, `@version`-generated record 
+ type constructors will discard any non-schema-required fields provided by the caller. When upgrading 
+ code that formerly "implicitly extended" a given schema version by propagating non-required fields, it 
+ is advisable to instead explicitly declare a new extension of the schema version to capture the propagated 
+ fields as required fields; or, if it makes more sense for a given use case, one may instead define a new 
+ schema version that adds these propagated fields as required fields directly to the schema (likely 
+ declared  as `::Union{Missing,T}` to allow them to be missing).

tidbits to draw attention to:

  • subtle nitpick, but since legolas doesn't really do anything at the table-level yet (besides validating the values/types of a table's rows), i defensively try to always use "fields" instead of "columns" just to avoid confusion (though the latter isn't necessarily incorrect depending on your perspective)

  • the former advice could be misunderstood to mean "add a new required field to an existing schema version" (though it might not have intended that) but doing so would be a breaking change which necessitates a schema version bump

  • generally the preferred path in Legolas v0.5 is to translate implicit/dynamic extensions into explicit/static extensions wherever reasonable, though in some cases introducing that might entail a tradeoff between "thrust things into the Right Way on Legolas v0.5" and the amount of breakage you're willing to propagate that may or may not be worthwhile depending on the scenario

@ericphanson ericphanson merged commit 4d49630 into main Nov 22, 2022
@ericphanson ericphanson deleted the eph/upgrade branch November 22, 2022 17:35
kleinschmidt pushed a commit to beacon-biosignals/LegolasFlux.jl that referenced this pull request Nov 23, 2022
* Add upgrade note to README

requires beacon-biosignals/Legolas.jl#71; currently points to preview docs, but we should land that, then update the link here to point to dev or stable docs.

xref #21

* fix link
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.

4 participants