Skip to content

Type helper cleanup, library reorganizations - #21

Merged
kevmoo merged 6 commits into
masterfrom
type_helper_more
Jul 24, 2017
Merged

Type helper cleanup, library reorganizations#21
kevmoo merged 6 commits into
masterfrom
type_helper_more

Conversation

@kevmoo

@kevmoo kevmoo commented Jul 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Reorganize libraries so there is one clear import for each use case.
Support nesting in TypeHelper.
Cleanup the API for TypeHelper
Other misc

@kevmoo
kevmoo force-pushed the type_helper_more branch 3 times, most recently from 324ec97 to 8818d8f Compare July 22, 2017 20:40
@kevmoo kevmoo changed the title WIP: Type helper cleanup Type helper cleanup Jul 23, 2017
@kevmoo kevmoo changed the title Type helper cleanup Type helper cleanup, library reorganizations Jul 23, 2017
@kevmoo
kevmoo requested a review from natebosch July 23, 2017 00:29
@kevmoo

kevmoo commented Jul 23, 2017

Copy link
Copy Markdown
Collaborator Author

@natebosch it's big, but it's really nice

CC @jakemac53

@kevmoo kevmoo mentioned this pull request Jul 23, 2017
Merged
kevmoo added 4 commits July 23, 2017 13:06
@kevmoo
Prefer annotated overrides, except for example files
b950ef4
@kevmoo
Fix parameter names for overrides
4ae1d45
@kevmoo
Eliminate depth notion for nested objects
704905d 
You can reuse the same name – `e` in our case – at each level
It makes reading the generated code a bit harder – so use tools
@kevmoo
Simplify TypeHelper
524cb74
@kevmoo
kevmoo force-pushed the type_helper_more branch from 68b250a to fe93e28 Compare July 23, 2017 20:08
@kevmoo

kevmoo commented Jul 23, 2017

Copy link
Copy Markdown
Collaborator Author

@natebosch – rebased on latest

natebosch
natebosch approved these changes Jul 23, 2017
View reviewed changes

@natebosch natebosch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would definitely be easier to review in smaller increments... but overall looks good.

Comment thread CHANGELOG.md Outdated

@natebosch natebosch Jul 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[nit] why the blank lines? (just realized they've been in there from the start...)

@kevmoo kevmoo Jul 24, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

A bit easier to read the source and the parsed markdown.

Comment thread lib/src/json_serializable_generator.dart Outdated

@natebosch natebosch Jul 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[nit] maybe a comment or a more descriptive variable name?

@kevmoo kevmoo Jul 24, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

Comment thread lib/src/json_serializable_generator.dart Outdated

@natebosch natebosch Jul 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it feel odd that there is both a _coreHelpers and a CoreHelper()...

@kevmoo kevmoo Jul 24, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. Also caught a bug! – CoreHelper should be in _coreHelpers!

Comment thread lib/src/json_serializable_generator.dart Outdated

@natebosch natebosch Jul 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

th?

@kevmoo kevmoo Jul 24, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

TypeHelper – would you prefer e?

@natebosch natebosch Jul 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe just h for 'helper' coming from _allHelpers -> or change it to _allTypeHelpers and keep th

Comment thread lib/src/type_helper.dart Outdated

@natebosch natebosch Jul 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[nit] could be typeArguments or typeArgumentsOf... get doesn't add value most of the time.

@kevmoo kevmoo Jul 24, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

nice!

Comment thread lib/src/type_helper.dart Outdated

@natebosch natebosch Jul 23, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[nit] Is the comment adding new information?

BTW a pattern I've seen Matan use with TypeChecker is to name the variable like $String which makes the usage look nicer than repeating TypeChecker

@kevmoo kevmoo Jul 24, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Uh...I think this is deleted code...

@kevmoo kevmoo Jul 24, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Nevermind. Found it. Uh...it makes it clear what it's for.

Better a trivial doc comment than no doc comment. This is a public API.

Not sure where you're going with the $String thing...

@kevmoo
kevmoo force-pushed the type_helper_more branch 2 times, most recently from 69d86d8 to 2eb9284 Compare July 24, 2017 16:47
@kevmoo
TypeHelper: Added (de)serializeNested arguments to (de)serialize
8fe4786 
Allows `Iterable`, `List`, and `Map` to be implemented as helpers
Allow rename and reorganize public libraries.
@kevmoo
kevmoo force-pushed the type_helper_more branch from 2eb9284 to 8fe4786 Compare July 24, 2017 16:55
@kevmoo
kevmoo merged commit 8fe4786 into master Jul 24, 2017
@kevmoo
kevmoo deleted the type_helper_more branch July 24, 2017 16:58
@Gopinathp Gopinathp mentioned this pull request Jan 14, 2020
Closed
@FabioPagano FabioPagano mentioned this pull request Nov 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@natebosch natebosch natebosch 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

@kevmoo @natebosch