Re: Thoughts for 5.8.2

On Tue, May 31, 2022 at 5:57 PM Volker Lamp <[email protected]> wrote:

> Hi guys

Hello! And thanks for creating this new thread and discussion! I was
very, very right when I said Volker would be a great addition to the
team and it's showing already. :)

> Pickung up what Thiago's wrote in another thread recently -
>
> > What about a release next week? I've just committed CORS support and
> > I'm looking forward to get it released. :)
>
> Here are my thoughts about the next release. Well done for adding CORS
> support, Thiago. It's a useful addition to Tapestry.

Thanks! Nothing complicated or fancy, but I guess it still can be useful. :)

> Further, we have a
> number of smaller bug fixes and improvements. I'm all for releasing
> early, however, I think we should make an effort towards including the
> fixes for the issues listed below also:
>
> TAP5-2718 Tapestry Announcement lacks version

I believe I've found the problem and I've just committed a fix.

> TAP5-2714 Checkbox component with Bootstrap 4

Something similar and pretty simple was done in GridPager: @Inject
org.apache.tapestry5.services.compatibility.Compatibility and call
compatibility.enabled(Trait.BOOTSTRAP_4) to check whether it's running
with Bootstrap 4 or not, then output the class attribute accordingly.

> TAP5-2721 Broken links to 5.8.1 asc files on download
>            page (more a todo than a commit)

I believe this will be a won't fix. It seems the .asc file generation
got broken or missing during the Gradle upgrade. I'm not sure these
files are actually needed according to ASF policies. If not, there's
already 2 other checksums and we can just ignore this from now on.

> The above are probably not very complicated or challenging. Fixing them
> can make a difference for new users, though  - no second chance for a
> first impression.

Agreed.

> TAP5-2713 ChecksumPath
> I understand Ben already fixed this one, so the issue could be set to
> closed also.

It seems to be fixed and has a very nice unit test. I've closed it. By
the way, the other day I was looking at tickets and there were a
number of them with fixes but apparently people forgot to update the
ticket accordingly.

> TAP5-2723 CronExpression fails handling L-<dayOfMonth>
> It would be nice if Charles didn't have to wait another few months for a
> fix.

We don't need to wait a few months to cut a new release. This has
happened due to lack of stuff to include in a release. Any committer
of the Tapestry team can do a release. It requires some effort,
specially the first time (setting up GnuPG, etc), and the process is
documented in the Tapestry site.

> That said, I agree with Ben: the code in CronExpression isn't easy
> to understand and hence the cause of the problem not easily spotted.

That's a huge understatement. :) I'd never looked at this class and,
whoa, that's complicated.

> My small contribution so far is a test case which I added to the TAP5-2723
> branch.

This is not a small contribution. It's a big one.

>  I won't have the time to work on it more during the next couple
> of weeks, so either somebody else picks it up or we just leave it until
> later.

I won't have enough time for that in the next few weeks either, so,
unless somebody else can pick it up, I vote for leaving it to later.

Another possibility is to check the original source: the first commit,
11 years ago (!!!) says the code is borrowed from Quartz. Our copy
hasn't been updated ever since, while I'd guess Quartz's one has been
changed a number of times since then:
https://github.com/quartz-scheduler/quartz/blob/master/quartz-core/src/main/java/org/quartz/CronExpression.java.

> Thanks,

Thank *you*!

>
> Volker
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>


-- 
Thiago