Skip to content
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

chore: split and organize files #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

chore: split and organize files #26

wants to merge 2 commits into from

Conversation

bradenhilton
Copy link
Member

This will probably break.

Unfinished and very WIP.

I would appreciate some feedback on my approach, as I don't really know what I'm doing (I'm learning a lot, though!) :)

@bradenhilton bradenhilton force-pushed the split-files branch 2 times, most recently from 14dc9e2 to c42d6eb Compare May 14, 2024 21:35
@bradenhilton
Copy link
Member Author

The workflow and CI changes should be in their own PR, I've just included them here temporarily.

I used pkg because I wanted it to be possible to only use the funcs for one or more types without using the entire library. I'm guessing this would also be possible by making everything internal and just having a New*FuncMap function for each func type.

Some specific questions:

  1. Is there a more idiomatic approach?
  2. If the packages and docs are split as expected, shouldn't the docs be further split so that a user can only use the docs they need? Should each func type package have its own docs? If so, what would that look like?
  3. Is there a preference between each package just providing a FuncMap vs a NewFuncMap function?

Copy link

@halostatue halostatue left a comment

Choose a reason for hiding this comment

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

LGTM, but I’m going to defer to Tom on this one.

@twpayne
Copy link
Member

twpayne commented May 16, 2024

Hmm, I'm negative on this. Reasons:

  • Moving functions into fine-grained packages isn't really the Go way. Go tends to prefer larger packages. Certainly, having a package contain only a single function (as is the case with booleanfuncs and stringfuncs) is certainly over-engineering.
  • The classification of functions is quite arbitrary. Isn't encoding/decoding also a kind of transform?
  • What is the actual problem solved by splitting the functions across several packages?

Copy link
Member

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

See previous comment. I need to know what problem is this splitting solves.

@bradenhilton
Copy link
Member Author

... Certainly, having a package contain only a single function (as is the case with booleanfuncs and stringfuncs) is certainly over-engineering.

True, but that is also partly because the library is unfinished.

The classification of functions is quite arbitrary. Isn't encoding/decoding also a kind of transform?

Indeed. I wanted to group functions mostly based on what they operate on or manipulate.

What is the actual problem solved by splitting the functions across several packages?

In addition to wanting to potentially offer each function group as a separate package for users, my primary concern is the lines of code, which you may not see as an issue, and which may demonstrate my inexperience with Go.

#16 shows that we have currently only scratched the surface of what we would like to achieve, and we still have almost 400 lines of code in a single file. What does that look like when we have enough functions for a release?

Many other template function libraries are structured in a similar way, though I would never use that as my primary motivation.

On reflection, it seems I don't have any particularly strong reasons for doing this. We can close and revisit this if/when it's necessary, if preferred.

@twpayne
Copy link
Member

twpayne commented May 16, 2024

Thanks for the reply. Maybe it's worth splitting the files but keeping everything in a single package for now?

I also think we should export the individual Go template functions with reasonable names (e.g. rename eqFoldTemplateFunc to EqFold) but this might be better done in a separate PR?

@bradenhilton
Copy link
Member Author

Maybe it's worth splitting the files but keeping everything in a single package for now?

I agree, but further to your point about arbitrariness, perhaps holding off on this would better inform us regarding how to group the functions? Naming things is hard, after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants