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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for multiple formats #2079
Add support for multiple formats #2079
Changes from 15 commits
698e991
71f4adc
d356ce8
7c5cde5
10d68ee
229aaab
9560ca0
9640e67
4bacfd9
eee9aec
1331e41
ac048cb
1170071
7bbba51
c44b5b3
e46c231
955a52e
6fb636a
87978c8
b55a94a
61a6d61
e77aecd
9777f38
69a377f
0270cba
e0f057f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extremely minor, but thoughts on using a kwarg for nice readability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My Personal Ruby Style Guide ™️ says to only use kwargs for > 2 arguments 🤷🏻
Regardless, this method is only used once, so I'm just going to inline it. No point in having default values for a single callsite anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting pretty complex, is this something we could break into more methods, or perhaps a separate class that's responsible for returning template objects that define the relevant methods? Like
name
,method_name
,format
, etc.?It might also make the testing story a bit easier and more comprehensive since we don't have to couple the compiler and template logic as heavily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man, all I wanted to do while writing this PR was refactor this entire file, but I held back. I'm happy to go down that route, but maybe it would be best to do as a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, the compiler has gotten a bit gnarly! Part of the reasoning was that I had a hard to following the code in here (but I also didn't have a ton of time to dedicate to reading it) so I thought refactoring might make it easier to follow the changes.
One strategy I've done in the past for changes like this is refactoring the PR on-top of the new behavior, backporting it while removing the new functionality as a separate PR (so 1-to-1 functionality wise), then re-adding those initial refactor changes on-top. It's a bit roundabout, but I found it helps reduce churn and makes the changes easy-to-follow.
I'm also fine with a follow-up, but I'd also say I haven't reviewed this code in depth yet if we wanted to go that route. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use
inspect
here?