Skip to content

Conversation

@dbarnett
Copy link
Contributor

@dbarnett dbarnett commented Jan 8, 2020

Creates helper functions for the boilerplate of common operations in formatter implementations: codefmt#formatterhelpers#Format({cmd}) and codefmt#formatterhelpers#AttemptFakeRangeFormatting({startline}, {endline}, {cmd}). These make conventions more obvious and help any intentional differences from the most common boilerplate version stand out more obviously.

Copy link
Member

@malcolmr malcolmr left a comment

Choose a reason for hiding this comment

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

LGTM


""
" @public
" Format lines in the current buffer via a formatter invoked by {cmd}. The
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps document that {cmd} is something you can pass to maktaba#syscall#Create()? As it stands, I wasn't sure if this was requiring a Syscall object, and the cases above where you passed a list were bugs.

(also below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good catch. It was a bug (but strangely wasn't caught by the tests… I'll need to investigate coverage there).

It actually does take a maktaba.Syscall… updated to clarify that and enforce.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I think there's some advantage in being able to pass in anything that can be passed to syscall#Create(), since then we can pass a list in the simple case, and a Syscall object in the uncommon case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah! Good idea, I forgot we made it that clever. And I guess it didn't fail tests because it wasn't broken. 🤦‍♂️

Updated to accept both.

Clarifies that the formatterhelper functions expect a maktaba.Syscall,
enforces that, and fixes two formatters buildifier and rustfmt that
didn't.
Copy link
Member

@malcolmr malcolmr left a comment

Choose a reason for hiding this comment

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

LGTM++

@dbarnett dbarnett merged commit 024911c into master Jan 8, 2020
@dbarnett dbarnett deleted the common_funcs branch July 29, 2020 03:31
@dbarnett dbarnett restored the common_funcs branch July 29, 2020 03:31
@dbarnett dbarnett deleted the common_funcs branch July 29, 2020 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants