Skip to content

chore(go): Refactor#39

Merged
candiduslynx merged 1 commit intomainfrom
chore/small-refactor
Jan 24, 2023
Merged

chore(go): Refactor#39
candiduslynx merged 1 commit intomainfrom
chore/small-refactor

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

@candiduslynx candiduslynx commented Jan 24, 2023 •
edited
Loading

2 enhancements:

  1. Save a bit if space (empty struct is 0, pointer is 4-8 bytes)
  2. Ease the init (no need to explicitly allocate transformers)

hermanschaaf
hermanschaaf reviewed Jan 24, 2023
View reviewed changes
@hermanschaaf
Copy link
Copy Markdown
Contributor

hermanschaaf commented Jan 24, 2023

What is the reason for the refactor? Why is the new version better?

bbernays
bbernays approved these changes Jan 24, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@bbernays bbernays left a comment

Choose a reason for hiding this comment

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

I am going to approve this but... In my opinion this change makes the code less readable/ easy to understand, so the tradeoff of readability vs saving a few bytes isn't really worth it... But will defer to the rest of the team

@erezrokah
Copy link
Copy Markdown
Member

erezrokah commented Jan 24, 2023 •
edited
Loading

Save a bit if space (empty struct is 0, pointer is 4-8 bytes)

I'm not sure this is a valid concern for us at the moment, unless we see this impacts the overall experience of the module. So maybe we should refactor this when we see the need?

Ease the init (no need to explicitly allocate transformers)

This is mostly a style issue we don't enforce (i.e. explicit init vs implicit init), so I could go either way. This PR has a side effect that CSV transformers are now available to the JSON client and vice versa, instead of being nil. I don't see it as a big concern though.

TLDR: I understand what this PR does and why, but I'm not sure the refactor is required

@candiduslynx candiduslynx merged commit 3631c88 into main Jan 24, 2023
@candiduslynx candiduslynx deleted the chore/small-refactor branch January 24, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbernays bbernays bbernays approved these changes

+1 more reviewer

@hermanschaaf hermanschaaf hermanschaaf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ignore-for-release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@candiduslynx @hermanschaaf @erezrokah @bbernays