search

aim42 / htmlSanityCheck

Standalone (batch- and command-line) and Gradle-plugin html sanity checker - detects missing images, dead links and cross-references, duplicate link targets (anchors) and the like.
Apache License 2.0
68 stars 42 forks source link

support passing a FileCollection #264

Closed jechlin closed 5 years ago

jechlin commented 5 years ago

Currently you can only pass a single root directory, plus list of files.

Using a gradle FileCollection would give the user much more control, as we could exclude certain directories.

gernotstarke commented 5 years ago

that's a good idea. I'm currently (too) busy with other things - but I'd very much like to support you, if you like to approach this issue...

thanx for your interest in htmlSC, Gernot

erdi commented 5 years ago

I'm looking into providing a PR with this. In my opinion this requires a bigger change and I would like to confirm with you, @gernotstarke, that you're happy with what I would like to do:

These are obviously breaking changes but I believe that they will provide a much more flexible mechanism for specifying which files to check compared to what's currently there thanks to using the outstanding finding and filtering capabilities of Gradle's FileCollection API.

gernotstarke commented 5 years ago

@erdi, thank you so much for your structured proposal: Yes, I'm perfectly happy with your suggestions, they sound plausible to me.

I'm honored to have you "on board"...

(with your massive experience in testing: in case you have any suggestions on how to better and/or more effectively test htmlSC - please feel free...)

regards, Gernot

erdi commented 5 years ago

Thanks for confirming that you’re happy with the proposed changes. Hopefully I will find some time tomorrow to make them and submit a PR.

I’m not sure what you mean by me “being on board” but we are using your sanity checker on a project at work and I’m just contributing back changes that we had to apply in a custom built artifact to be able to use it. I hate custom builds and forks and it makes me very happy that contributing back to the project has been so swift so far - thanks for being so responsive to issues and PRs.

With regards to testing I’m not sure I see anything standing out that would need changing. I was very positively surprised that an integration test using GradleRunner has already been in place as I’m used to writing one from scratch when contributing back to Gradle plugin projects.

On Sat, 9 Feb 2019 at 17:25, Dr. Gernot Starke notifications@github.com wrote:

@erdi https://github.com/erdi, thank you so much for your structured proposal: Yes, I'm perfectly happy with your suggestions, they sound plausible to me.

I'm honored to have you "on board"...

(with your massive experience in testing: in case you have any suggestions on how to better and/or more effectively test htmlSC - please feel free...)

regards, Gernot

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aim42/htmlSanityCheck/issues/264#issuecomment-462062801, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMii3YdNjuzJ2rRvzZtAErzsT2o-QVvks5vLwSbgaJpZM4ankEO .

thc202 commented 5 years ago

I'd suggest using ConfigurableFileCollection instead, to start making use of lazy configuration: https://docs.gradle.org/current/userguide/lazy_configuration.html

erdi commented 5 years ago

I might be missing something but as far as I understand what I suggested will not prevent you from assigning a ConfigurableFileCollection to HtmlSanityCheckTask.sourceDocuments if you so wish. I also don't see why my proposal would be at odds with using lazy configuration. Can you please elaborate, @thc202?

thc202 commented 5 years ago

That one can set a ConfigurableFileCollection into HtmlSanityCheckTask.sourceDocuments does not make it a lazy property (thus one is still with the same problems that lazy configuration meant to solve).

erdi commented 5 years ago

I find being able to use FileCollection API for specifying which files to check and changing properties on HtmlSanityCheckTask to org.gradle.api.provider.Property (as that is what I believe you are suggesting, @thc202, albeit in a very convoluted way) to be two orthogonal concerns.

thc202 commented 5 years ago

While I think using lazy properties everywhere (it makes sense) is the ultimate goal I was not suggesting that. I just suggested using ConfigurableFileCollection instead of FileCollection, it was a quick win, especially since the API is already being broken. (Granted I was not explicit on the whole changes required (i.e. making sourceDocuments final also) but linked to the documentation which shows that.)

gernotstarke commented 5 years ago

have you "on board"...

Marcin, by "on board" I meant that I feel honoured having you as contributor.

erdi commented 5 years ago

I think I now understand what you meant. Instead of making sourceDocuments a writable FileCollection you suggest making it a read only ConfigurableFileCollection initialized to a FileTree based on sourceDir. I will submit a PR with such change.

On Sun, 10 Feb 2019 at 17:00, thc202 notifications@github.com wrote:

While I think using lazy properties everywhere (it makes sense) is the ultimate goal I was not suggesting that. I just suggested using ConfigurableFileCollection instead of FileCollection, it was a quick win, especially since the API is already being broken. (Granted I was not explicit on the whole changes required (i.e. making sourceDocuments final also) but linked to the documentation which shows that.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aim42/htmlSanityCheck/issues/264#issuecomment-462151235, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMii2sRbdI6WYjterNFtn4JohSNLtbxks5vMFAogaJpZM4ankEO .