Enable Marshaling support#5
Open
eduncan911 wants to merge 8 commits into
Open
Conversation
Merged
eduncan911
force-pushed
the
feature-Enable-Unmarshaling-Support
branch
2 times, most recently
from
May 13, 2017 15:34
79dae0e to
eaeee5d
Compare
Repository owner
deleted a comment from
coveralls
Jul 3, 2017
eduncan911
changed the title
eduncan911
added a commit
that referenced
this pull request
Feb 7, 2020
# This is the 1st commit message: Go Mod and Vendor # This is the commit message #2: asd # This is the commit message #3: asd # This is the commit message #4: asd # This is the commit message #5: asd # This is the commit message #6: asd # This is the commit message #7: asd # This is the commit message #8: asd # This is the commit message #9: asd
This was referenced Oct 28, 2020
eduncan911
force-pushed
the
feature-Enable-Unmarshaling-Support
branch
from
November 4, 2020 21:14
91f287b to
8bcde0c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note, this PR is incomplete. Still needs:
I am creating this PR early in transparency for others to see and comment on while work continues.
Backwards Compatibility
While this is targeting a 2.x point release, I am trying to keep it backwards compatible with the existing 1.x releases.
So far it remains backwards compatible as long as you used the API methods in 1.x (
podcast.New(...),p.AddItem(...),i.AddEnclosure(...), etc etc). These do not change at all from outside perspective, validated by all unit and integration tests (100% code coverage baby!).What does break is the removal of specialized fields that were suffixed with "Formatted." For example,
Enclosure.TypeFormatted,Enclosure.LengthFormatted,Item.AuthorFormatted,Item.PudDateFormattedand so on. They are all now removed. These were originally added because, frankly, I was too lazy to implement theMarshalinterfaces when I originally wrote it - it was a simple hack to just use a 2nd field and manipulate the result during the API calls. These were only used if you bypassed the API methods and attempted to manipulate the Podcast feeds yourself.So basically, if you stuck to the API methods you were fine. But, if you used the structs only and did everything manually, you could see some code breakage.
Marshaling
I actually came up with a use case lately that I needed to compare previous feeds with newly generated ones (to limit updates). Therefore, I needed a way to read my old generated feed and compare it to the new feed - while ignoring things like PubDate that changes on every invocation. To do this I needed to read an existing feed - and what better way to do this, than when this Podcast package!
Except, it didn't work... I left out some common Marshaling requirements such as XMLName-ing the nil-able fields of structs and had a lot of custom marshaling going on with the Formatted fields.
With the changes to use the
MarshalerandUnmarshalerinterfaces, I needed to remove those "Formatted" fields. I think this was enough to warrant a full on code review and Major release bump to 2.x.A bit of Freedom Removed
This unfortunately creates a delima: you no longer have full control over your Feed with the removal of the plaintext "Formatted"
stringfields from the structs, and the move to useMarshalerandUnmarshalerinterfaces. Now, you must use the structs and set items with strongly-typed objects.To me, this is a no brainer - i want the
MarshalerandUnmarshalerinterfaces to handle compliant feeds as well as strongly-typed objects.But to others that wanted complete control over their feeds, with non-compliant specially "Formatted" fields such as their own PubDate format, this 2.x breaks that ability.
I believe this is ok. But, I am open to discussions here.
Too Strict on
Enclosure.EnclosureType?Unmarshaling, where we had none before have you, is implemented in a very strict nature with this version.
See the large comment block in
enclosure.goline L122: https://github.com/eduncan911/podcast/compare/develop...feature-Enable-Unmarshaling-Support?expand=1#diff-b2f071ccd78abc2ac116c7a8665bde7eR122For now, I am keeping it strict. Feel free to discuss.