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

Move parse package internally and modify Makefile #132

Merged
merged 5 commits into from
Sep 7, 2024
Merged

Conversation

mfridman
Copy link
Owner

@mfridman mfridman commented Sep 7, 2024

Fix #131

This PR does a few things:

  • moves the ./parse package into ./internal/parse
  • add tools module to track golangci-lint
  • minor updates to Makefile

@mfridman mfridman changed the title Mf/gh131 Move parse package internally and modify Makefile Sep 7, 2024
@mfridman mfridman merged commit a507644 into main Sep 7, 2024
3 checks passed
@mfridman mfridman deleted the mf/gh131 branch September 7, 2024 16:46
Copy link
Contributor

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

LGTM 👍

One question, you mention

add tools module to track golangci-lint

I didn't find this one. I saw changes in Makefile, but that's it.

Is it what you were talking about?

I thought I would see something about toolchain entry in go.mod or something like that

@egonelbre
Copy link
Contributor

Any specific reason parse package was moved to internal? It's been quite helpful to write custom tools for analyzing/processing the test output.

@mfridman
Copy link
Owner Author

mfridman commented Sep 9, 2024

Any specific reason parse package was moved to internal? It's been quite helpful to write custom tools for analyzing/processing the test output.

Trying to get this project to a v1 state I wanted to refactor and make some breaking changes to that package. It was quite coupled to the tool itself so I want to clean that up a bit.

Happy to move it out of ./internal though if you think there's some use for it.

@mfridman
Copy link
Owner Author

mfridman commented Sep 9, 2024

LGTM 👍

One question, you mention

add tools module to track golangci-lint

I didn't find this one. I saw changes in Makefile, but that's it.

Is it what you were talking about?

I thought I would see something about toolchain entry in go.mod or something like that

I was experimenting with it (189a957), but at the end decided the project was way too simple so I reverted those changes and forgot to update the description. Ultimately waiting for golang/go#48429 to land to improve the story around tool deps.

@egonelbre
Copy link
Contributor

Happy to move it out of ./internal though if you think there's some use for it.

Yeah, it does come up occasionally for me.

I sometimes use some other "internal" to indicate that it's not finalized, like github.com/mfridman/tparse/exp/parse or github.com/mfridman/tparse/private/parse.

But, yeah, I can keep using the older version if you want to clean up the parse package.

@mfridman
Copy link
Owner Author

mfridman commented Sep 9, 2024

Thanks for surfacing it, happy to move it back to the top level. There might be some fairly minor breaking changes, but my ultimate goal is to make it an off-the-shelf parsing library and nail down the returned results.

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.

Move parse package internally
3 participants