Skip to content

Add include filter #670

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add include filter #670

wants to merge 1 commit into from

Conversation

LMG
Copy link
Collaborator

@LMG LMG commented Mar 26, 2022

It can be inserted in workspace files, to allow for composition

@LMG LMG force-pushed the include_filter branch from 66ad3f3 to 9e8f57a Compare March 26, 2022 06:38
@LMG
Add include filter
ff8bb64 
It can be inserted in workspace files, to allow for composition
Add tests
@LMG LMG force-pushed the include_filter branch from 9e8f57a to ff8bb64 Compare March 26, 2022 08:59
@LMG LMG requested a review from christian-schilling March 26, 2022 08:59
@christian-schilling
Copy link
Member

A few things:

  • It looks like there is quite some duplicated code from the workspace filter implementation. Could this already be factored out?
  • Could the workspace filter even be expressed in terms of the include filter? (As an alias for :[:/wsdir,:include=wsdir/workspace.josh]
  • I would prefer to not specify the .josh file extension in the filter, so that there is a stronger convention arround naming the files that way.

@christian-schilling christian-schilling marked this pull request as draft October 28, 2022 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christian-schilling christian-schilling Awaiting requested review from christian-schilling

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LMG @christian-schilling