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

Resource filtering fix #226

Merged
merged 4 commits into from
Jul 15, 2016

Conversation

abelsromero
Copy link
Member

This PR includes the fixes discussed in issue #187.
The required libraries to enable filtering have been added and now it's possible to use it. Hovewer, this is not exposed in the documentation.

Here is the list of changes in this PR:

  • Updated README to align with maven-resource-plugin configuration and explain the supported options. Note that using only without works also, I guess it's my gradle side that preffers it.
  • Added integration test for property filtering (a.k.a. replacing). Even if it's not officially supported, that way we have an example if someone wants to use it and asks about it.
  • Fixed an issue: when defining custom file extensions in sourceDocumentExtension, these were also copied. Now they are excluded.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 59.56% when pulling 3579c9d on abelsromero:resource_filtering_fix into a9c3f7b on asciidoctor:master.

@coveralls
Copy link

coveralls commented Jun 25, 2016

Coverage Status

Coverage increased (+0.3%) to 60.212% when pulling 3579c9d on abelsromero:resource_filtering_fix into a9c3f7b on asciidoctor:master.

@abelsromero
Copy link
Member Author

comments?

<include>**/*.jpg</include>
<include>**/*.gif</include>
<exclude>**/.svn</exclude>
<!-- (Optional) Directory to copy to. By default uses the option `sourceDirectory` -->
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this comment is correct. The resources aren't copied to the source directory but rather the output directory, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Reading it again I think that the overall section is not clear. I propose the next text.

  • Replaced first line telling that if resources are not set all resources insidesourceDirectoryare copied
  • Corrected descriptions for and
resources:: list of resource files to copy to the output directory (e.g., images, css). The configuration follows the same patterns as the `maven-resources-plugin`. If not set, all resources inside `sourceDirectory` are copied
+
[source, xml]
----
    <resources>
        <resource>
            <!-- (Mandatory) Directory to copy from. Paths are relative to maven's ${baseDir} -->
            <directory>DIRECTORY</directory>
            <!-- (Optional) Directory to copy to. By default uses the option `outputDirectory` -->
            <targetPath>OUTPUT_DIR</targetPath>
            <!-- (Optional) NOTE: SVN, GIT and other version control files are excluded by default, there's no need to add them -->
            <excludes>
                <exclude>**/.txt</exclude>
            </excludes>
            <!-- (Optional) If not set, includes all files but default exceptions mentioned -->
            <includes>
                <include>**/*.jpg</include>
                <include>**/*.gif</include>
            </includes>
        </resource>
        <resource>
                ...
    <resources>
----

@abelsromero
Copy link
Member Author

Sorry to ask, but what do you think of the proposals?

@coveralls
Copy link

coveralls commented Jul 9, 2016

Coverage Status

Coverage increased (+0.3%) to 60.212% when pulling 8afe558 on abelsromero:resource_filtering_fix into a9c3f7b on asciidoctor:master.

@abelsromero
Copy link
Member Author

@mojavelinux I am a bit confused, I see the 'thumbs' up in the issue, but since it does not show the dat I am not sure if this has been review.

Is it good to merge?

@mojavelinux
Copy link
Member

I didn't think about the fact that the reaction icon doesn't show the date. When I'm giving a 👍 to say "proceed with merge" I'll use a comment instead. In this case, I was giving the thumbs up to merge.

@abelsromero
Copy link
Member Author

Good to know I did not screw up 😄

@mojavelinux
Copy link
Member

mojavelinux commented Sep 3, 2016 via email

@abelsromero abelsromero deleted the resource_filtering_fix branch September 16, 2017 08:24
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.

None yet

3 participants