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

Expose methods for listing files #102

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

PawkyPenguin
Copy link

@PawkyPenguin PawkyPenguin commented Apr 21, 2018

Hey, thanks for the awesome plugin!

I picked up where #35 left off (new PR because I'm missing write access) and implemented your suggestion about exposing files for a single projection.

All-in-all, three functions are exposed for more or less granular listings of files:

  • list_project_files() returns a dict containing files for the whole active project
  • list_files_for_projection(projection) returns a dict of files matching a single projection. The projection given as the argument has to be identical to a projection as defined in the config and only the first projection found is returned. I thought about passing a pattern as argument to match multiple projections but that's a bit too crazy imo.
  • list_files_for_category(category) returns a list of files of a single "category" (I stuck with the term that Expose a method to list commands and scoped files #35 used but it might make more sense to call it "type" now that I think about it - I can adjust that if you'd like)

I added the third function because I imagine that a user might want to get a list of all his "test"s and do something with them.

If you agree with the changes so far I can add some help docs for them.

@tpope
Copy link
Owner

tpope commented Apr 21, 2018

I don't understand list_files_for_projection(). If you only care about one individual glob, why involve projectionist at all?

@PawkyPenguin
Copy link
Author

I mostly implemented it because of the suggestion you made in #35 but maybe that got outdated over the years. I think projectionist has the advantage that projections only work in certain scope. Say for instance someone working on a project has defined projections for *.scala and *.java in that project, but projections for *.java and *.class files in another project, and projections for *.java and *.png files in yet another project. The user could call list_files_for_projection three times with these three projections and agreggate the results into a list to perform some operation (like packing them into a jar). Projectionist takes care that the jar only contains scala and java files for the first, java and class for the second, and java and images for the third project.

I agree that having to call the function three times is clunky though... It makes more sense to turn the argument into a list of projections. It could also be beneficial to add projection properties to the output, i.e. the alternate file, the console to run etc. - I could pack that into a second PR, this one is already on the large side. What do you think?

@tpope
Copy link
Owner

tpope commented Apr 21, 2018

I can only assume that I meant a single type, not a single projection key.

I don't really understand your example. It sounds like you are saying you decide if PNG files make it into the archive based on whether someone defined a *.png projection, regardless of what that projection contains. Does that mean if you don't want PNG files in your archive, you are forbidden from using *.png for any other reason?

@PawkyPenguin
Copy link
Author

The example was fictional and I couldn't come up with a better idea.

I can only assume that I meant a single type, not a single projection key

Yeah, I've only been using list_files_for_category so far. Then we can probably remove the other method because I can't think of a concrete workflow either. If someone wanted to list files that belong to a projection he could just give the projection a unique type.

I'll rename list_files_for_category to list_files_for_type to keep consistency, and remove list_files_for_projection. I'm gonna add some helpdocs later today.

Remove function to list projections
@PawkyPenguin PawkyPenguin force-pushed the master branch 2 times, most recently from d91e03d to 961f3ff Compare April 24, 2018 02:33
@PawkyPenguin
Copy link
Author

Sorry, got sidetracked. I just amended the commit from before to include the mentioned changes. Documentation has been updated as well.

Fix help coloring

Fix duplicate helptag
@tpope
Copy link
Owner

tpope commented May 14, 2018

  • Ditch projectionist#list_project_files(). I'm not convinced of its value and it's easy to roll yourself if you need it.
  • Rename list_files_for_type to files_for_type.
  • Inline get_projection_from_variant into get_projections_from_variants and files_in_projections into files_for_type. I don't find the names to be helpful enough to justify separate single use functions.
  • Move the docs into comments.

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.

2 participants