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

fix: allow well-known headers #40

Merged
merged 2 commits into from
May 9, 2024
Merged

fix: allow well-known headers #40

merged 2 commits into from
May 9, 2024

Conversation

lasiar
Copy link
Owner

@lasiar lasiar commented May 9, 2024

See: #12

@lasiar lasiar force-pushed the fix/v1/initialism branch 2 times, most recently from e77068d to 39690ed Compare May 9, 2024 10:21
@lasiar lasiar force-pushed the fix/v1/initialism branch from 39690ed to 57eee15 Compare May 9, 2024 10:22
@lasiar lasiar merged commit a8968c0 into v1 May 9, 2024
4 checks passed
"WWW-Authenticate",
"X-WebKit-CSP",
"X-Real-IP",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have defined only the ones where result would be different than well known. But I would have put all of them in the tests for testing possible regression.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's what you did in initialismer in fact, so I'm unsure why you have the well known list then 🤔

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't want to find manually non-canonical headers in the well-know list

@@ -0,0 +1,74 @@
// Code generated by initialismer; DO NOT EDIT.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get the purpose of this file. It's for tests only no?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, only tests, the linter shouldn't print diagnostics.

In v2, I will add the golden file and invert this test:

h.Get("Caldav-Timezones")  -> h.Get("CalDAV-Timezones")

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I totally missed it was a file in testdada

@@ -5,3 +5,8 @@ test:

linter:
golangci-lint -v run ./...

generate:
go run ./cmd/initialismer/*.go -target="mapping" > ./initialism.go
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure why you are not using go generate with a `//go:generate in the code

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for replying to every single comment I did.

This one has no reply yet

Copy link
Owner Author

Choose a reason for hiding this comment

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

In my opinion:

  • go generate -- generate non repo files
  • make generate -- generate repo files

I didn't think much about it 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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


genTarget, err := parseTarget()
if err != nil {
slog.Error("parse target:", slog.Any("error", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

No exit or return here?

Copy link
Owner Author

@lasiar lasiar May 9, 2024

Choose a reason for hiding this comment

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

Oh, thanks, wil fix this

@ccoVeille
Copy link
Contributor

@lasiar I'm unsure if you saw my comments. I'm sorry for the ping/notification if you did

@ldez ldez mentioned this pull request May 9, 2024
@lasiar lasiar deleted the fix/v1/initialism branch July 14, 2024 17:55
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.

2 participants