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.

Sign up for GitHub

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

Jump to bottom

Add txtpbfmt #298

Merged
merged 8 commits into from
Jun 1, 2023
Merged

Add txtpbfmt #298

merged 8 commits into from
Jun 1, 2023

Conversation

pat-trunk-io
Copy link
Contributor

No description provided.

@pat-trunk-io
Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@pat-trunk-io pat-trunk-io requested a review from TylerJang27 May 31, 2023 01:25
@pat-trunk-io
Copy link
Contributor Author

@TylerJang27
I got it to work with trunk fmt, but the jest tests don't seem to work.
The output in the snapshot is the same as the source file

@pat-trunk-io
Copy link
Contributor Author

@TylerJang27 I got it to work with trunk fmt, but the jest tests don't seem to work. The output in the snapshot is the same as the source file

Nevermind, figured it out.
Ready for review

TylerJang27
TylerJang27 approved these changes May 31, 2023
View reviewed changes
Copy link
Collaborator

@TylerJang27 TylerJang27 left a comment

Choose a reason for hiding this comment

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

Looks good! See a couple comments, I'm particularly worried about the semver issue, although I'll investigate more tomorrow. If possible, hold off on merging til then

linters/txtpbfmt/plugin.yaml
batch: true
formatter: true
suggest_if: files_present
known_good_version: 0.0.0-20230412060525-fa9f017c0ded
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know what this version is supposed to represent/how stable is it?

Additionally, there are some cases in which we don't support semver pre-release tags other than alpha, beta, and rc. I'll need to play around with this a bit to make sure it doesn't break anything in the trunk upgrade flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't have tags sadly

linters/txtpbfmt/txtpbfmt.test.ts
@@ -0,0 +1,3 @@
import { linterFmtTest } from "tests";

linterFmtTest({ linterName: "txtpbfmt", namedTestPrefixes: ["test0", "test1"] });
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: namedTestPrefixes should be unnecessary here, but fine to leave it

@TylerJang27
Copy link
Collaborator

@pat-trunk-io Just did a quick audit. This version tag should be fine. If we need to manually update people, we can bump the known_good_version and people will correctly pick it up during trunk upgrade. And since their repo doesn't have tagged releases, we don't have to worry about that logic. The one thing that this will not support is versioned subcommands, so that's something we'll need to keep in mind if it becomes relevant.

Going to try and fix the other plugins tests so this can land.

@pat-trunk-io pat-trunk-io merged commit bd3c0b9 into main Jun 1, 2023
@pat-trunk-io pat-trunk-io deleted the 05-31-Add_txtpbfmt branch June 1, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerJang27 TylerJang27 TylerJang27 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pat-trunk-io @TylerJang27