Skip to content

caddyfile: Implement variadics for import args placeholders#5249

Merged
mholt merged 5 commits intomasterfrom
import-snippet-line-number
Feb 17, 2023
Merged

caddyfile: Implement variadics for import args placeholders#5249
mholt merged 5 commits intomasterfrom
import-snippet-line-number

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 francislavoie force-pushed the import-snippet-line-number branch from 38ee1df to c028b35 Compare February 15, 2023 10:25
@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