Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Enhancement: The encoder, but cleaner this time (hopefully) #4142

Merged
merged 8 commits into from
Mar 15, 2022
Merged

Enhancement: The encoder, but cleaner this time (hopefully) #4142

merged 8 commits into from
Mar 15, 2022

Conversation

haltman-at
Copy link
Contributor

@haltman-at haltman-at commented Jun 25, 2021 •
edited
Loading

The contents of this PR are identical to #3974, so please see that for a description, including questions I would like resolved.

What's different between this PR and #3974 is that I've reorganized it into just 6 commits, to make reviewing it easier. Hope this is helpful @gnidan!

Note that I have not yet made any of the changes I proposed in #3974, since I was waiting for review first. Also uh I guess I never really fixed any typedoc problems, I (or rather @gnidan) just got it to the point where the typedoc compiles, but I just remembered I never actually bothered checking out the results and fixing problems there, so I may still need to do that...

Edit: Addresses issues #2189 and #3819

@haltman-at haltman-at requested a review from gnidan June 25, 2021 07:25
@haltman-at haltman-at force-pushed the wrap2 branch 3 times, most recently from 8a34790 to 4de59df Compare July 1, 2021 00:22
@haltman-at haltman-at mentioned this pull request Jul 1, 2021
Closed
gnidan
gnidan suggested changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

This review is for just the first commit, since there's likely a lot of discussion to be had even just with the codec changes. Mostly, I'm very keen on getting rid of the long series of nested if statements everywhere. Let's do away with that pattern.

@haltman-at haltman-at force-pushed the wrap2 branch 3 times, most recently from 7ffe7c2 to d29b325 Compare July 8, 2021 23:26
@haltman-at haltman-at force-pushed the wrap2 branch 2 times, most recently from b7a466d to bbd7e37 Compare July 15, 2021 22:48
@haltman-at haltman-at force-pushed the wrap2 branch 2 times, most recently from e9278af to 81da9d9 Compare July 29, 2021 00:36
@haltman-at haltman-at mentioned this pull request Jul 29, 2021
Merged
@haltman-at haltman-at force-pushed the wrap2 branch 3 times, most recently from 1926b90 to 417f02f Compare July 31, 2021 04:53
@haltman-at haltman-at mentioned this pull request Jul 31, 2021
Closed
@haltman-at haltman-at force-pushed the wrap2 branch 3 times, most recently from 28893e4 to c6af744 Compare August 6, 2021 21:12
@haltman-at haltman-at force-pushed the wrap2 branch 2 times, most recently from a24beb0 to 14d1141 Compare August 12, 2021 23:09
@haltman-at
Copy link
Contributor Author

Heh, I only just noticed that maybe watchMappingKey / constructSlot should also accept Strings in place of strings when specifying struct members...

@haltman-at haltman-at mentioned this pull request Aug 17, 2021
Merged
@haltman-at haltman-at force-pushed the wrap2 branch 2 times, most recently from 0a2c077 to ad23150 Compare August 27, 2021 20:02
@haltman-at haltman-at force-pushed the wrap2 branch 2 times, most recently from 5546115 to c5dd5bc Compare January 20, 2022 23:20
@haltman-at haltman-at force-pushed the wrap2 branch 3 times, most recently from 0415ca5 to 69fe3ac Compare February 3, 2022 23:32
@haltman-at haltman-at force-pushed the wrap2 branch 3 times, most recently from 4bd9577 to 63caa2c Compare February 17, 2022 22:57
@haltman-at haltman-at force-pushed the wrap2 branch 3 times, most recently from 64fba7d to 6de92b6 Compare February 24, 2022 22:04
gnidan
gnidan approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Just went through the docs and played with this. I think this is good to merge :)

@haltman-at
Copy link
Contributor Author

OK, well I'm going to go back and split up that test file before actually merging this. :P

@haltman-at haltman-at force-pushed the wrap2 branch 2 times, most recently from 4fe0c3a to 79253e1 Compare March 10, 2022 09:01
@haltman-at
Copy link
Contributor Author

OK, I've now split up the tests -- the resulting files are still pretty big, but, it's no longer totally ridiculous. So, if you want to look any of that over again...

gnidan
gnidan approved these changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Test splitup looks nice :)

@haltman-at haltman-at merged commit 5910517 into develop Mar 15, 2022
@haltman-at haltman-at deleted the wrap2 branch March 15, 2022 20:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gnidan gnidan gnidan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@haltman-at @gnidan