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

Implement wildcards on the file list. #180

Merged
merged 4 commits into from
Mar 8, 2021

Conversation

Gui13
Copy link
Contributor

@Gui13 Gui13 commented Feb 8, 2021

This is done by searching for the '*' string in the file path, which is a sub-group of the whole "file glob" capabilities of Golang.

Fixes #176

Note: I believe we could add a full-glob support with regexes and so on, but this would mean either:

  • passing the entire file names systematically through the filepath.Glob() regardless of their content
  • or adding a flag telling the configuration to Glob() the files

The first is relatively easy, but I didn't want to go "full glob" if that's not desirable (which I'm not quite equiped to judge).
Your call though, I'm up to coding it.

This is done by searching for the '*' string in the file path, which is
a sub-group of the whole "file glob" capabilities of Golang.
Copy link
Owner

@martin-helmich martin-helmich left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! In general, 👍 from me. I've just added a few comments on some function signatures that I think could be streamlined and some things I feel should be pointed out in the documentation before merging this issue.

As to the "full glob support" question that you've raised in your initial comment -- I'm not leaning in any particular direction right now. I'd be fine with leaving it as-is for now (and add full-glob support later, if feature requests should arise); OTOH just passing every file name through Glob should also be relatively inexpensive (since glob expansion is only done during startup, and not continuously -- however, see also my discussion on that) and should also not break BC in any way.

config/struct_namespace.go Outdated Show resolved Hide resolved
README.adoc Show resolved Hide resolved
config/loader_test.go Show resolved Hide resolved
@Gui13
Copy link
Contributor Author

Gui13 commented Feb 26, 2021

Hi @martin-helmich

Thanks for the comments, I agree with you on all of them, let's keep the scope simple right now.
For the dynamic addition of new files matching, I agree that monitoring the wildcard would involve a lot of things (inotify on all directories + filter for matching patterns), so I'll amend the readme with the limitations rather than launch a big implementation.

I've been swamped at work the last 2 weeks so I'll go back to this maybe next week.

@Gui13
Copy link
Contributor Author

Gui13 commented Feb 26, 2021

Ok I took the time to mount up the error. Tell me if the current state of the PR is ok for you!

Copy link
Owner

@martin-helmich martin-helmich left a comment

Choose a reason for hiding this comment

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

Code looks good to me. 👍 Pending the documentation update, I'll happily merge this. 🙂

Wildcard are only evaluated at program start, I added a bit of documentation about that.
@codeclimate
Copy link

codeclimate bot commented Mar 8, 2021

Code Climate has analyzed commit d06cb06 and detected 0 issues on this pull request.

View more on Code Climate.

@Gui13
Copy link
Contributor Author

Gui13 commented Mar 8, 2021

Hi @martin-helmich ,

Sorry I got taken up by my reald world :)

I modified the documentation to add details about the glob being only evaluated at program start.

@martin-helmich
Copy link
Owner

👍 Thanks for the contribution!

@martin-helmich martin-helmich merged commit 97f7677 into martin-helmich:master Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Glob for files
2 participants