Issue #104 inlineHeader not working

[ppalaga]

No description provided.

[ppalaga]

@mathieucarbou I see that you added the in progress label 3 days ago, but I needed a fix quickly. This works for me, however, I admit I have not looked if there are tests covering inlineHeader or even added any.

[mathieucarbou]

Hi,

Thanks a lot for the PR! Yes I tried the feature and saw that it didn't work because it still required the <header> tag to be there.

Before merging your PR, could you make sure:

1. that it is tested
2. that reformatting is disabled (because it will be hard to follow the little changes in the history)

Thanks!

NB: (for github auto-close): this PR closes #104

[ppalaga]

@mathieucarbou 047ae19 brings a better design, no unintentional re-formating but still no tests.

[mathieucarbou]

ok! I'll wait until I have some time (or you) to add some tests before merging this,
Thanks a lot!

[ppalaga]

@mathieucarbou 3f2f801 :

• Moves the newly added class HeaderSource to com.mycila.maven.plugin.license.header package
• Adds a few tests of HeaderSource
• I do not think more tests are necessary, because HeaderSource hides the difference between headers defined via inlineHeader and header params and therefore the existing tests covering Header should be enough to prove that Header works properly with either kinds of HeaderSource.

[ppalaga]

@mathieucarbou is this good to be merged?

[mathieucarbou]

Hi @ppalaga ,

I was actually referring to a test that triggers a plugin execution, with the parameter inlineHeader set but not the parameter header. This is to mimic the real-life scenario that didn't work.

Actually, before your development, I think you should have started first by writing this failing test to verify that the problem is really fixed ;-) I was thinking of a test similar to this one: https://github.com/mycila/license-maven-plugin/blob/master/license-maven-plugin/src/test/java/com/mycila/maven/plugin/license/CheckTest.java

This sort of test really triggers a full plugin execution like a human does, and this will verify that the feature works, and failed before.

I am a bit "severe" on this issue, because I know the feature is not working and thus it requires a complete test to prove it works now (and not before) and works as a regression test also. The test should also proves that without your changes, it does not pass: this is important so that we are sure to fix the right issue...

Thanks :-)

[ppalaga]

@mathieucarbou added two Mojo tests in fd347b4 . Enough like that?

[mathieucarbou]

Yes :-) Thanks!

[ppalaga]

@mathieucarbou thanks for merging, is there a good hope for a release soon?

[mathieucarbou]

Yes :-)
I just want to wait a little first to see if @mirabilos comes up with something for issue #38 and his PR #115.
But I plan to make the 3.0 release no later than by the end of next week (Sun. 28th). There is a lot of enhancements in this milestone! Thanks a lot for your help :-)

[ppalaga]

Thanks, end of the next week sounds reasonable (but not later, please).

[mirabilos]

Mathieu Carbou dixit:

I just want to wait a little first to see if @mirabilos comes up with

Sorry, got ill the last two days, and I’m on a conference and won’t
be back at work before next Tuesday, so if you can go ahead without
any input from me in the meantime, please do so.

[mathieucarbou]

Ok! I'll do the release this week-end then :-)