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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a file filter based on regular expressions #1186

Merged
merged 25 commits into from Jan 20, 2015

Conversation

Projects
None yet
2 participants
@mried
Collaborator

mried commented Dec 29, 2014

The file filter is included within the ihate plugin and uses the new import_task_created event which is fired for each task after it is created.
The plugin is only a first approach. Maybe the plugin rules are much to complicated. I'm not even sure if I'll use them for my own library...
A new pipeline stage is used to fire the event so the code to be changed is limited to one place.


Note: Running tox -e py27cov or even nose on my windows box lets fail all the new tests of test_ihate:

beets: WARNING: ** error loading plugin ihate
beets: WARNING: Traceback (most recent call last):
  File "D:\work\beets\beets\beets\plugins.py", line 211, in load_plugins
    for obj in getattr(namespace, name).__dict__.values():
AttributeError: 'module' object has no attribute 'ihate'

Running the test module without the other modules is working. I found out the the paths in beetsplug.__path__ are different but couldn't figure out what's leading to the difference nor why one of values leads to failures. Seems I'm at the end of my python knowledge 馃槈...

Show outdated Hide outdated test/test_ihate.py Outdated
@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Dec 29, 2014

Member

This is looking great! Thank you for adding the appropriate event and everything.

I don't have access to a Windows box, so I'm a bit mystified about your test issues. I have one guess above鈥攍et me know if that works.

Would you mind expanding the documentation for the ihate plugin with the new option?

Member

sampsyo commented Dec 29, 2014

This is looking great! Thank you for adding the appropriate event and everything.

I don't have access to a Windows box, so I'm a bit mystified about your test issues. I have one guess above鈥攍et me know if that works.

Would you mind expanding the documentation for the ihate plugin with the new option?

Show outdated Hide outdated beetsplug/ihate.py Outdated
Show outdated Hide outdated beetsplug/ihate.py Outdated
Show outdated Hide outdated beetsplug/ihate.py Outdated
@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Dec 29, 2014

Member

The above few comments get at a simplification I wanted to suggest if this is going to be part of ihate: it seems like we may not need separate configuration options for stuff that can be encoded inside the regular expression. If that's the case, it could make the feature simpler to explain by making it symmetric with the rest of ihate: just use something like

ihate:
    path: regex_goes_here

to skip stuff matching the expression.

If this feels too strained to merge the new stuff with how ihate's configuration already works, it would still be 100% OK to reverse course and make a new filefilter plugin after all (and my apologies if I led you down the wrong path).

Member

sampsyo commented Dec 29, 2014

The above few comments get at a simplification I wanted to suggest if this is going to be part of ihate: it seems like we may not need separate configuration options for stuff that can be encoded inside the regular expression. If that's the case, it could make the feature simpler to explain by making it symmetric with the rest of ihate: just use something like

ihate:
    path: regex_goes_here

to skip stuff matching the expression.

If this feels too strained to merge the new stuff with how ihate's configuration already works, it would still be 100% OK to reverse course and make a new filefilter plugin after all (and my apologies if I led you down the wrong path).

@mried

This comment has been minimized.

Show comment
Hide comment
@mried

mried Dec 30, 2014

Collaborator

I've simplified the configuration of the ihate plugin as you suggested. The reason I created a lot of extra config options is simple: I always struggle with those regular expression extras like (?!...) or (?i) 馃槈. But you're right, we should keep the configuration as simple as possible.

I'm not sure about the album and singleton option. Right now you can use something like the following to configure different regular expressions for album and singleton imports:

album:
    path: <regex>
singleton:
    path: <regex>

Since the path config is the only possible option below album and singleton, we could combine them like:

albumpath: <regex>
singletonpath: <regex>

What do you think?

The documentation I left out at the first run is also added. I didn't want to document all the stuff which is removed afterwards 馃槅...

BTW: Feel free to change the docs. My English isn't native, so my texts might sound strang for other people...


The test cases still fail on windows. I'm totally ok with that since a lot of other test cases fail, too. Running the test case without the others brings a good result.

Collaborator

mried commented Dec 30, 2014

I've simplified the configuration of the ihate plugin as you suggested. The reason I created a lot of extra config options is simple: I always struggle with those regular expression extras like (?!...) or (?i) 馃槈. But you're right, we should keep the configuration as simple as possible.

I'm not sure about the album and singleton option. Right now you can use something like the following to configure different regular expressions for album and singleton imports:

album:
    path: <regex>
singleton:
    path: <regex>

Since the path config is the only possible option below album and singleton, we could combine them like:

albumpath: <regex>
singletonpath: <regex>

What do you think?

The documentation I left out at the first run is also added. I didn't want to document all the stuff which is removed afterwards 馃槅...

BTW: Feel free to change the docs. My English isn't native, so my texts might sound strang for other people...


The test cases still fail on windows. I'm totally ok with that since a lot of other test cases fail, too. Running the test case without the others brings a good result.

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Dec 30, 2014

Member

Great! Thank you for obliging my instinct to simplify, and for writing the docs (which look great, BTW). It's true that the tricks I mentioned can be hard to remember鈥攎aybe those would be worth adding to the docs for the benefit of forgetful people like you and me? 馃槂

Since the path config is the only possible option below album and singleton, we could combine them

Good point. Collapsing these seems like a great idea. I might use underscores in the names, but I'll leave it up to you. 馃悷

The test cases still fail on windows. I'm totally ok with that since a lot of other test cases fail, too. Running the test case without the others brings a good result.

OK, good to know. Yes, we need to do more digging into the tests on Windows鈥 someday I'll have the time to spin up a VM again for this purpose.

There's one last niggling concern that I want to think about here: the placement of the import_task_created event. That pipeline stage is definitely convenient, but there are a couple of drawbacks:

  • This will "miss" tasks created in the group_albums stage when that's enabled, since it comes after normal task creation.
  • Similarly, it will miss tasks created in the "mini-pipelines" in user_query when the user chooses the "as tracks" or "as albums" option.
  • Pipeline stages are mapped on to OS threads, and it seems like a tiny bit of a waste of resources to have a whole thread stage that might do nothing in every import pipeline.

With that in mind, what do you think about emitting the event inside ImportTask's constructor instead? Or somewhere similar, like just after constructing the object everywhere we do that? It's definitely less pretty, but it might be easier to explain in the long run.

Member

sampsyo commented Dec 30, 2014

Great! Thank you for obliging my instinct to simplify, and for writing the docs (which look great, BTW). It's true that the tricks I mentioned can be hard to remember鈥攎aybe those would be worth adding to the docs for the benefit of forgetful people like you and me? 馃槂

Since the path config is the only possible option below album and singleton, we could combine them

Good point. Collapsing these seems like a great idea. I might use underscores in the names, but I'll leave it up to you. 馃悷

The test cases still fail on windows. I'm totally ok with that since a lot of other test cases fail, too. Running the test case without the others brings a good result.

OK, good to know. Yes, we need to do more digging into the tests on Windows鈥 someday I'll have the time to spin up a VM again for this purpose.

There's one last niggling concern that I want to think about here: the placement of the import_task_created event. That pipeline stage is definitely convenient, but there are a couple of drawbacks:

  • This will "miss" tasks created in the group_albums stage when that's enabled, since it comes after normal task creation.
  • Similarly, it will miss tasks created in the "mini-pipelines" in user_query when the user chooses the "as tracks" or "as albums" option.
  • Pipeline stages are mapped on to OS threads, and it seems like a tiny bit of a waste of resources to have a whole thread stage that might do nothing in every import pipeline.

With that in mind, what do you think about emitting the event inside ImportTask's constructor instead? Or somewhere similar, like just after constructing the object everywhere we do that? It's definitely less pretty, but it might be easier to explain in the long run.

@mried

This comment has been minimized.

Show comment
Hide comment
@mried

mried Dec 31, 2014

Collaborator

The config options are simplified.

Emitting the event within the ImportTask constructor is dangerous: When creating a SingletonImportTask, the object will not be set up completely before the event is fired because the constructor of the deriving class (SingletonImportTask in this case) might not be completely executed.

So I tried to send the event after each creation of ImportTask, SingletonImportTask and ArchiveImportTask. What about the SentinelImportTask? Do we have to fire the event here, too?
There are also some places outside beets itself:

  • beetsplug/bench.py/match_benchmark
  • Some tests. I guess the event should not be fired here so we can ignore these files.

Hopefully I found all places...

Collaborator

mried commented Dec 31, 2014

The config options are simplified.

Emitting the event within the ImportTask constructor is dangerous: When creating a SingletonImportTask, the object will not be set up completely before the event is fired because the constructor of the deriving class (SingletonImportTask in this case) might not be completely executed.

So I tried to send the event after each creation of ImportTask, SingletonImportTask and ArchiveImportTask. What about the SentinelImportTask? Do we have to fire the event here, too?
There are also some places outside beets itself:

  • beetsplug/bench.py/match_benchmark
  • Some tests. I guess the event should not be fired here so we can ignore these files.

Hopefully I found all places...

@mried

This comment has been minimized.

Show comment
Hide comment
@mried

mried Dec 31, 2014

Collaborator

Hmm... I guess there is a drawback using this new type of event creation instead of the pipeline stage: I just tried to implement an automatic singleton detection plugin using the new event (#1167) . Using a pipeline stage to submit the event should give the ability to change the data which flows down the pipeline, shouldn't it? Hence the plugin could change the task from a single ImportTask to several SingletonImportTasks. That's not possible with the new solution, I guess (I'm also not sure if it worked using the pipeline stage event creation... 馃槙) Maybe this new event won't work for this at all since event handlers return values are ignored?
If it's not possible to change the data inside the pipeline using events, I think we should include this nevertheless and search another solution for the singleton detection to work.

Collaborator

mried commented Dec 31, 2014

Hmm... I guess there is a drawback using this new type of event creation instead of the pipeline stage: I just tried to implement an automatic singleton detection plugin using the new event (#1167) . Using a pipeline stage to submit the event should give the ability to change the data which flows down the pipeline, shouldn't it? Hence the plugin could change the task from a single ImportTask to several SingletonImportTasks. That's not possible with the new solution, I guess (I'm also not sure if it worked using the pipeline stage event creation... 馃槙) Maybe this new event won't work for this at all since event handlers return values are ignored?
If it's not possible to change the data inside the pipeline using events, I think we should include this nevertheless and search another solution for the singleton detection to work.

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Jan 1, 2015

Member

Interesting issues. Thank you so much for being thorough in checking out all the options. You've definitely done this well, but you're absolutely right that this approach is a little limiting (and we could still prevent some use cases鈥攚hich I think is also a problem with the old pipeline-stage version too).

I agree that we should just add the event if we can't find something perfect, but it would be even cooler to find one consistent interface that works for everything here. Here are two final ideas to consider:

  • Do some refactoring to make sure all tasks are produced in the ImportTaskFactory. This would require some cleanup to centralize the logic. Then we provide an event that's only emitted from ImportTaskFactory.tasks. This would also require that the event handler be able to return new tasks, also, which is something events can't currently do but something I'm interested in adding anyway. (I'll help with that if we choose this option.)
  • Add an alternative to the current plugin-provided pipeline stages that adds new stages at this point in the process. early_import_stage or something? We'd then have to resolve the other issues above: making sure these appear in the mini-pipelines, etc.

I think I lean toward the first option, even though it will require some nearer-term refactoring before we can get it working. Any thoughts?

Member

sampsyo commented Jan 1, 2015

Interesting issues. Thank you so much for being thorough in checking out all the options. You've definitely done this well, but you're absolutely right that this approach is a little limiting (and we could still prevent some use cases鈥攚hich I think is also a problem with the old pipeline-stage version too).

I agree that we should just add the event if we can't find something perfect, but it would be even cooler to find one consistent interface that works for everything here. Here are two final ideas to consider:

  • Do some refactoring to make sure all tasks are produced in the ImportTaskFactory. This would require some cleanup to centralize the logic. Then we provide an event that's only emitted from ImportTaskFactory.tasks. This would also require that the event handler be able to return new tasks, also, which is something events can't currently do but something I'm interested in adding anyway. (I'll help with that if we choose this option.)
  • Add an alternative to the current plugin-provided pipeline stages that adds new stages at this point in the process. early_import_stage or something? We'd then have to resolve the other issues above: making sure these appear in the mini-pipelines, etc.

I think I lean toward the first option, even though it will require some nearer-term refactoring before we can get it working. Any thoughts?

@mried

This comment has been minimized.

Show comment
Hide comment
@mried

mried Jan 1, 2015

Collaborator

I played around a bit and tried to imitate the behaviour of group_albums which does the same thing I want do do (add new tasks to the pipeline). The plugin is uploaded here (beside other plugins 馃槃): https://github.com/mried/beetsplug/blob/autosingleton/beetsplug/autosingleton.py

The idea is simple: If a false album task is detected, the action of the album task is set to skip. New SingletonImportTasks are emitted to the pipeline using pipeline.multiple(...). But that seems not to work at all 馃槥.

But maybe this is a third idea: Provide a way for plugins to add more tasks to the pipeline. I'm completely unsure if this is possible since I didn't understand the pipeline code right now 馃槙.

Anyway: Your second idea came into my mind, too. But I thought about a more general solution: A pipeline stage should know its dependent stages (not before and not after). This way, the pipeline will be set up dynamically.

The first solution is a very flexible approach, hence with a lot of work 馃槈. It would be much better if the ImportTaskFactory is responsible for the creation of all ImportTasks. But I don't see how tasks can provide all needed tasks. Right now it's a method to return all needed tasks for an import of files. What about the tasks created elsewhere, like in group_albums or user_query? Maybe I just didn't understand you right...

BTW: Happy new year! 馃帀

Collaborator

mried commented Jan 1, 2015

I played around a bit and tried to imitate the behaviour of group_albums which does the same thing I want do do (add new tasks to the pipeline). The plugin is uploaded here (beside other plugins 馃槃): https://github.com/mried/beetsplug/blob/autosingleton/beetsplug/autosingleton.py

The idea is simple: If a false album task is detected, the action of the album task is set to skip. New SingletonImportTasks are emitted to the pipeline using pipeline.multiple(...). But that seems not to work at all 馃槥.

But maybe this is a third idea: Provide a way for plugins to add more tasks to the pipeline. I'm completely unsure if this is possible since I didn't understand the pipeline code right now 馃槙.

Anyway: Your second idea came into my mind, too. But I thought about a more general solution: A pipeline stage should know its dependent stages (not before and not after). This way, the pipeline will be set up dynamically.

The first solution is a very flexible approach, hence with a lot of work 馃槈. It would be much better if the ImportTaskFactory is responsible for the creation of all ImportTasks. But I don't see how tasks can provide all needed tasks. Right now it's a method to return all needed tasks for an import of files. What about the tasks created elsewhere, like in group_albums or user_query? Maybe I just didn't understand you right...

BTW: Happy new year! 馃帀

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Jan 2, 2015

Member

Hmm鈥 I'm not sure why pipeline.multiple wouldn't work for you. We use that a few times in the importer鈥攁ny additional detail about what exactly happens instead?

And no, you're quite right that this much centralization might be too hard to achieve. But even so, something that reduces the number of places that we construct tasks would help with the overall agenda of letting task construction be customizable. I don't see a perfect solution at the moment, so perhaps some creativity that has yet to dawn on us will be helpful鈥

Member

sampsyo commented Jan 2, 2015

Hmm鈥 I'm not sure why pipeline.multiple wouldn't work for you. We use that a few times in the importer鈥攁ny additional detail about what exactly happens instead?

And no, you're quite right that this much centralization might be too hard to achieve. But even so, something that reduces the number of places that we construct tasks would help with the overall agenda of letting task construction be customizable. I don't see a perfect solution at the moment, so perhaps some creativity that has yet to dawn on us will be helpful鈥

@mried

This comment has been minimized.

Show comment
Hide comment
@mried

mried Jan 2, 2015

Collaborator

Ah! I have to feed the result of pipeline.multiple to the pipeline afterwards. I thought this is a class method doing so for me 馃槥. So what happened when using my plugin? Exactly nothing 馃槃.

I'll try to move all the task generation into the ImportTaskFactory and prepare a mechanism to change them by a plugin event.

Collaborator

mried commented Jan 2, 2015

Ah! I have to feed the result of pipeline.multiple to the pipeline afterwards. I thought this is a class method doing so for me 馃槥. So what happened when using my plugin? Exactly nothing 馃槃.

I'll try to move all the task generation into the ImportTaskFactory and prepare a mechanism to change them by a plugin event.

@mried

This comment has been minimized.

Show comment
Hide comment
@mried

mried Jan 14, 2015

Collaborator

I've merged the recent changes and it still works 馃槂. What else needs to be done on this topic?

Collaborator

mried commented Jan 14, 2015

I've merged the recent changes and it still works 馃槂. What else needs to be done on this topic?

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Jan 14, 2015

Member

Awesome; thank you for updating. I'm sorry for disappearing recently; beets has been really active lately and I've been low on time. I'm going to oscillate back to this as soon as I can鈥擨 think this is very nearly good to go if not ready already.

Member

sampsyo commented Jan 14, 2015

Awesome; thank you for updating. I'm sorry for disappearing recently; beets has been really active lately and I've been low on time. I'm going to oscillate back to this as soon as I can鈥擨 think this is very nearly good to go if not ready already.

Show outdated Hide outdated beets/importer.py Outdated
@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Jan 18, 2015

Member

I'm starting now by merging the refactoring pieces first. (I'm hoping that this will make each piece of the puzzle easier to reason about.) I plan to open a new PR that isolates the plugin hook piece next.

Member

sampsyo commented Jan 18, 2015

I'm starting now by merging the refactoring pieces first. (I'm hoping that this will make each piece of the puzzle easier to reason about.) I plan to open a new PR that isolates the plugin hook piece next.

@mried

This comment has been minimized.

Show comment
Hide comment
@mried

mried Jan 18, 2015

Collaborator

I've implemented the "execute all archive tasks and only use the path of the last task" solution but I'm still not sure if this is a good idea... Let me know what you think.

Edit: Whoops, I didn't notice this silly for task in tasks[0] bug... 馃槼

Collaborator

mried commented Jan 18, 2015

I've implemented the "execute all archive tasks and only use the path of the last task" solution but I'm still not sure if this is a good idea... Let me know what you think.

Edit: Whoops, I didn't notice this silly for task in tasks[0] bug... 馃槼

Instead of using the list of archive tasks for further importing, onl鈥
鈥 the fist task was used - which is not iterable, of course.

sampsyo added a commit that referenced this pull request Jan 18, 2015

Event handlers can now return values
Part of #1186, which I hope to break into smaller pieces.

sampsyo added a commit that referenced this pull request Jan 18, 2015

Centralize some counting in ImportTaskFactory
Part of a larger effort to simplify read_tasks for plugin interposition
(#1186).

sampsyo added a commit that referenced this pull request Jan 18, 2015

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Jan 18, 2015

Member

Alright鈥擨've landed a compromise solution in dc5a79e.

That piece adds the import_task_created event in a much simpler way. It's more limited, but it should be sufficient for this new feature. There's no more useful return value from the event鈥攕ince you've opted for using the "set skip" option anyway, I don't think we need it. This makes the modifications to importer.py much smaller鈥攖here are only four functions where tasks are created, so we just need calls in four places:

  • read_tasks
  • query_tasks
  • user_query's "as tracks" generator
  • group_albums

Does this all look OK to you? If so, I'll get started on porting the rest of the PR over.

Member

sampsyo commented Jan 18, 2015

Alright鈥擨've landed a compromise solution in dc5a79e.

That piece adds the import_task_created event in a much simpler way. It's more limited, but it should be sufficient for this new feature. There's no more useful return value from the event鈥攕ince you've opted for using the "set skip" option anyway, I don't think we need it. This makes the modifications to importer.py much smaller鈥攖here are only four functions where tasks are created, so we just need calls in four places:

  • read_tasks
  • query_tasks
  • user_query's "as tracks" generator
  • group_albums

Does this all look OK to you? If so, I'll get started on porting the rest of the PR over.

@mried

This comment has been minimized.

Show comment
Hide comment
@mried

mried Jan 20, 2015

Collaborator

Beside commit b7125e4 (see my comment there), everything looks fine for the simple filter (which is all I added to the ihateplugin). Plugins are able to skip a task.

Can I help somehow on this task?

Collaborator

mried commented Jan 20, 2015

Beside commit b7125e4 (see my comment there), everything looks fine for the simple filter (which is all I added to the ihateplugin). Plugins are able to skip a task.

Can I help somehow on this task?

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Jan 20, 2015

Member

Great! I'm glad this seems to be what we need.

I'd be happy to have your help if you're not burned out on contributing to this feature. 馃槂 As capricious and unnecessary as it seems, I've been thinking more about this as I reviewed it further and I'm no longer so sure that merging with ihate was such a brilliant idea in the first place. In retrospect, the purposes are substantially different鈥攁nd it might be better, long-term, for your new functionality to live in its own small, focused plugin. How would you feel about going back on that decision? Would that be too frustrating of a reversal?

Member

sampsyo commented Jan 20, 2015

Great! I'm glad this seems to be what we need.

I'd be happy to have your help if you're not burned out on contributing to this feature. 馃槂 As capricious and unnecessary as it seems, I've been thinking more about this as I reviewed it further and I'm no longer so sure that merging with ihate was such a brilliant idea in the first place. In retrospect, the purposes are substantially different鈥攁nd it might be better, long-term, for your new functionality to live in its own small, focused plugin. How would you feel about going back on that decision? Would that be too frustrating of a reversal?

@mried

This comment has been minimized.

Show comment
Hide comment
@mried

mried Jan 20, 2015

Collaborator

Great! I'm glad this seems to be what we need.

As long as we go on and add the rest of the PR 馃槈

The filter stuff is placed at a separate plugin like it was at the beginning of the PR. And: no, it's not frustrating as long as we are not in a loop 馃槂

Collaborator

mried commented Jan 20, 2015

Great! I'm glad this seems to be what we need.

As long as we go on and add the rest of the PR 馃槈

The filter stuff is placed at a separate plugin like it was at the beginning of the PR. And: no, it's not frustrating as long as we are not in a loop 馃槂

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Jan 20, 2015

Member

Great! I was thinking and now I believe that we should merge it with ihate again JUST KIDDING SORRY

Thank you for sticking with this for so long. I'll merge this now. 馃帀

Member

sampsyo commented Jan 20, 2015

Great! I was thinking and now I believe that we should merge it with ihate again JUST KIDDING SORRY

Thank you for sticking with this for so long. I'll merge this now. 馃帀

@sampsyo sampsyo merged commit a62a152 into beetbox:master Jan 20, 2015

sampsyo added a commit that referenced this pull request Jan 20, 2015

Revert #1186 changes to beets core
The changes were:
- Return values from events.
- A new `import_task_created` event.
Both were added preemptively to master.

sampsyo added a commit that referenced this pull request Jan 20, 2015

sampsyo added a commit that referenced this pull request Jan 20, 2015

sampsyo added a commit that referenced this pull request Jan 20, 2015

Fix filefilter (#1186) tests for consistent colons
I added this to the pretend output a few commits ago.

sampsyo added a commit that referenced this pull request Jan 20, 2015

sampsyo added a commit that referenced this pull request Jan 20, 2015

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Jan 20, 2015

Member

Okay, all merged up!

I hope this wasn't too presumptive of me, but I renamed the plugin to just filefilter. I think this makes the purpose a little bit clearer.

Thank you again for all your effort! You're a true hero.

Member

sampsyo commented Jan 20, 2015

Okay, all merged up!

I hope this wasn't too presumptive of me, but I renamed the plugin to just filefilter. I think this makes the purpose a little bit clearer.

Thank you again for all your effort! You're a true hero.

sampsyo added a commit that referenced this pull request Jan 20, 2015

@mried

This comment has been minimized.

Show comment
Hide comment
@mried

mried Jan 21, 2015

Collaborator

Great! 馃憤

But I guess I got you wrong some comments ago. I thought you wanted to add the "task replacement by plugins feature" as well to make it possible to implement #1167. Is this still an option?

Collaborator

mried commented Jan 21, 2015

Great! 馃憤

But I guess I got you wrong some comments ago. I thought you wanted to add the "task replacement by plugins feature" as well to make it possible to implement #1167. Is this still an option?

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Jan 21, 2015

Member

No, you read me right鈥擨 just wanted to take this one step at a time. We can figure out how to evolve this minimally for the other issue.

Member

sampsyo commented Jan 21, 2015

No, you read me right鈥擨 just wanted to take this one step at a time. We can figure out how to evolve this minimally for the other issue.

@mried mried deleted the mried:import-filefilter branch Feb 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment