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

Support %, %%, #, ## operators #6

Closed

Conversation

pecigonzalo
Copy link

@pecigonzalo pecigonzalo commented Jul 12, 2019

Add support for more interpolations

Notes

Relates to #2

export FOO="bar"
echo '${FOO:-buzz}' | go run ./cmd/interpolate/main.go
This will make it easier to expand and support other operators
@pecigonzalo pecigonzalo force-pushed the feature/more-operators branch 2 times, most recently from c0751d4 to d65473d Compare July 12, 2019 12:17
@pecigonzalo pecigonzalo changed the title WIP: Support more operators Support %, %%, #, ## operators Jul 12, 2019

go 1.12

require github.com/drone/envsubst v1.0.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to require this?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, its used by substitutions.go

The official Go behaves a bit differently, here is the diff:

diff --git i/this.go w/this.go
index d39d244..9306b0c 100644
--- i/this.go
+++ w/this.go
@@ -6,14 +6,13 @@ package path
 
 import (
 	"errors"
-	"strings"
 	"unicode/utf8"
 )
 
-// ErrBadPattern indicates a pattern was malformed.
+// ErrBadPattern indicates a globbing pattern was malformed.
 var ErrBadPattern = errors.New("syntax error in pattern")
 
-// Match reports whether name matches the shell pattern.
+// Match reports whether name matches the shell file name pattern.
 // The pattern syntax is:
 //
 //	pattern:
@@ -43,7 +42,10 @@ Pattern:
 		star, chunk, pattern = scanChunk(pattern)
 		if star && chunk == "" {
 			// Trailing * matches rest of string unless it has a /.
-			return !strings.Contains(name, "/"), nil
+			// return !strings.Contains(name, "/"), nil
+
+			// Return rest of string
+			return true, nil
 		}
 		// Look for match at current position.
 		t, ok, err := matchChunk(chunk, name)
@@ -59,8 +61,7 @@ Pattern:
 		}
 		if star {
 			// Look for match skipping i+1 bytes.
-			// Cannot skip /.
-			for i := 0; i < len(name) && name[i] != '/'; i++ {
+			for i := 0; i < len(name); i++ {
 				t, ok, err := matchChunk(chunk, name[i+1:])
 				if ok {
 					// if we're the last chunk, make sure we exhausted the name
@@ -158,9 +159,6 @@ func matchChunk(chunk, s string) (rest string, ok bool, err error) {
 			}
 
 		case '?':
-			if s[0] == '/' {
-				return
-			}
 			_, n := utf8.DecodeRuneInString(s)
 			s = s[n:]
 			chunk = chunk[1:]

@lox
Copy link
Contributor

lox commented Jul 19, 2019

Aside from the github.com/drone/envsubst require, this looks really good. Thanks for your time and patience @pecigonzalo!

Just checking, are all these new expansions standard bash ones?

@pecigonzalo
Copy link
Author

They are, check: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02 and https://www.tldp.org/LDP/abs/html/parameter-substitution.html

I was going to add ^^ and ^ but I was not so sure about those

@lox
Copy link
Contributor

lox commented Jul 21, 2019

I'm uncomfortable bringing in github.com/drone/envsubst/path. They are an open-source competitor and it doesn't seem fair to them.

Besides that, if we were going to bring in that dependency, we should just move entirely to that library and contribute upstream.

@pecigonzalo
Copy link
Author

I understand, I would say that file is not the main thing tho, as its 3 lines modified on the file upstream, I would have to basically do the same and copy the file here. I thought it would be better to give them credit by referencing their project than re-implement it here.

I think it would be a great idea to "merge" the projects and just contribute to that one, I suggested using drone/envsubst directly over in Slack, but since that is a major change Im not sure how to PR that here, at least for the functions referenced here: https://github.com/buildkite/interpolate/pull/6/files#diff-b6d44dcddb0b7ad9e5118697cc8ddb3eR10 which I also tried to use from their upstream, but the package does not export them.

drone/envsubst its a bit more simple in the way it works (eg. does not have a VariableExpansion type/struct implementation) and many of their functions are not exported.

@pecigonzalo
Copy link
Author

@lox what would be your preferred approach here?

@pecigonzalo
Copy link
Author

Any updates on this?

@pecigonzalo
Copy link
Author

Merged master to resolve conflicts.

@pecigonzalo
Copy link
Author

@ticky @lox Any chance to merge this? It has been waiting for over 1yr :)

@lox
Copy link
Contributor

lox commented Nov 22, 2020

I'm not too fussed about merging this in here, the complexity is that we'd need to match the same rules in Buildkite's backend parser. @yob thoughts?

@moskyb moskyb deleted the branch buildkite:master May 30, 2024 06:38
@moskyb moskyb closed this May 30, 2024
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.

3 participants