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

caddyfile: Implement variadics for import args placeholders #5249

Merged
merged 5 commits into from
Feb 17, 2023

Conversation

WeidiDeng
Copy link
Member

imported snippets reflect actual lines in file

fix 5202

@francislavoie
Copy link
Member

Oooh, fun. Can you show an example Caddyfile with this?

@mholt
Copy link
Member

mholt commented Dec 12, 2022

Very cool, thanks for working on this!

We might need a test case for this, or at least an example that we know works. There's a number of test failures, currently.

@mholt mholt added the under review 🧐 Review is pending before merging label Dec 12, 2022
@francislavoie
Copy link
Member

Hmm. I'm not sure how I feel about the : syntax. Yes obviously it's very Go-like, but I think it reads weird when following a . dot. I think we need to consider some other syntax options.

@WeidiDeng
Copy link
Member Author

Hmm. I'm not sure how I feel about the : syntax. Yes obviously it's very Go-like, but I think it reads weird when following a . dot. I think we need to consider some other syntax options.

Just {args[:]} ?

@francislavoie
Copy link
Member

Hmm, maybe. I think if we want to use the [] syntax, we should maybe make {args[0]} work as well (maybe deprecate {args.0}?)

@mholt
Copy link
Member

mholt commented Dec 13, 2022

Personally, I'm ok with the : syntax 🤷‍♂️

Copy link

@rudSarkar rudSarkar left a comment

Choose a reason for hiding this comment

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

Is it possible to add a name for each test case like the below example given?

{
	name: "Test duplicate import",
	input: `(t1) {
		abort {args.:}
	}
	:8080 {
		import t1
		import t1 true
	}`,
	errorText: "Caddyfile:6(import t1):2",
},

@francislavoie francislavoie added the feature ⚙️ New feature or request label Feb 6, 2023
@francislavoie francislavoie added this to the v2.6.3 milestone Feb 6, 2023
@francislavoie francislavoie changed the title implement variadic placeholders caddyfile: Implement variadics for import args placeholders Feb 6, 2023
@mholt
Copy link
Member

mholt commented Feb 8, 2023

@rudSarkar That sounds like a good/useful idea. Could be done either in this PR or a later commit.

@mholt mholt modified the milestones: v2.6.3, v2.6.4 Feb 8, 2023
@rudSarkar
Copy link

@mholt I think It will be better to add this on later commit.

@mholt mholt modified the milestones: v2.6.4, v2.6.5 Feb 14, 2023
WeidiDeng and others added 5 commits February 15, 2023 02:10
imported snippets reflect actual lines in file
- Moved the import args handling to a separate file
- Using {args[0:1]} syntax now
- Deprecate {args.*} syntax
- Use a replacer map for better control over the parsing
- Add plenty of warnings when invalid placeholders are detected
- Renaming variables, cleanup comments for readability
- More tests to cover edgecases I could think of
- Minor cleanup to snippet tracking in tokens, drop a redundant boolean field in tokens
@francislavoie
Copy link
Member

I worked on cleaning this up -- see recent commit for details.

@mholt
Copy link
Member

mholt commented Feb 16, 2023

I think this is looking good!

I might wait for #5275 though, so as to not introduce another conflict. Thanks for working on this both of you 😊

@francislavoie
Copy link
Member

francislavoie commented Feb 16, 2023

I don't think they'll conflict. This only affects the Caddyfile and that PR affects runtime usage of the replacer.

Both do use : in their syntax, but this PR happens during the Caddyfile adapt, on the raw token text, and isn't passed through the replacer (unless a warning happens and an invalid args placeholder falls through to the config, but that's besides the point IMO).

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

You're right - on closer inspection the files do not overlap. Let's merge this and give it a try!

@mholt mholt removed the under review 🧐 Review is pending before merging label Feb 16, 2023
@mholt mholt modified the milestones: v2.6.5, v2.7.0 Feb 16, 2023
@mholt mholt merged commit 8bc05e5 into master Feb 17, 2023
@mholt mholt deleted the import-snippet-line-number branch February 17, 2023 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get placeholder array from index
4 participants