Skip to content

tiltfile: add a stringable type to make it easier to unpack strings#3698

Merged
nicks merged 1 commit into
masterfrom
nicks/stringable
Aug 13, 2020
Merged

tiltfile: add a stringable type to make it easier to unpack strings#3698
nicks merged 1 commit into
masterfrom
nicks/stringable

Conversation

@nicks

@nicks nicks commented Aug 12, 2020

Copy link
Copy Markdown
Contributor

Hello @maiamcc, @landism,

Please review the following commits I made in branch nicks/stringable:

0422a04 (2020-08-12 19:35:49 -0400)
tiltfile: add a stringable type to make it easier to unpack strings

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@nicks
nicks requested review from landism and maiamcc August 12, 2020 23:40

@landism landism 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.

LGTM, definite improvement over status quo.

This highlights to me that we don't have much consistency about whether a function's string parameter has to be an actual string or simply a stringable. Think it'd make sense to have our unpacker swap out all strings with stringables before calling the starlark unpacker?

@maiamcc maiamcc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ooh nice

@nicks

nicks commented Aug 13, 2020

Copy link
Copy Markdown
Contributor Author

@landism ya, possibly! one thing I went back and forth on is if we should have a string Unpacker that trims the whitespace/newlines off blobs, or matches them against some validity regexp, or if that would be confusing

@nicks
nicks merged commit cf9ac37 into master Aug 13, 2020
@nicks
nicks deleted the nicks/stringable branch August 13, 2020 19:56
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.

3 participants