Skip to content

Conversation

line-o
Copy link
Member

@line-o line-o commented Apr 9, 2025

This is #66 rebased on current master

@line-o line-o requested review from a team April 9, 2025 22:16
Copy link
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

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

Failing test

@line-o
Copy link
Member Author

line-o commented Apr 9, 2025

Yes, but hard to tell what is failing in latest. It passes fine in exist 6.

@duncdrum
Copy link
Contributor

duncdrum commented Apr 9, 2025

1) should find article with extended markdown contents and code highlighting

Not sure what is hard to tell. It passes with the typecast

@line-o
Copy link
Member Author

line-o commented Apr 9, 2025

I assume it is not related to the change in this PR but rather a change in latest exist in the last 3 month

@duncdrum
Copy link
Contributor

@line-o Ci runs from today are all green on main and :latest I still think the typecast is functionally necessary. It's possible that this is an instance of a bug in the typing system, or a poor function descriptor somewhere. But it looks very much related

@line-o
Copy link
Member Author

line-o commented Apr 10, 2025

I am not sure what test runs you are referring to @duncdrum
Could you link them?

I have to add that I built function-documentation from master and installed it manually on an instance built on latest develop
#72 is the result of that test

@line-o
Copy link
Member Author

line-o commented Apr 10, 2025

@line-o
Copy link
Member Author

line-o commented Apr 10, 2025

even the released version 1.2.1 is broken on exist 7
I am wondering what is tested in CI and why this PR is the one where it starts failing. My luck, I guess.

@duncdrum
Copy link
Contributor

@line-o

wget http://exist-db.org/exist/apps/public-repo/public/templating-1.1.0.xar -O 001.xar
the test is still running with 1.1

@line-o
Copy link
Member Author

line-o commented Apr 10, 2025

Yes, and it should pass.
See, the change is about removing a cast to boolean that is completely unnecessary.
The type is already enforced by the function signature in line 101. $details MUST be an xs:boolean

app:print-module($module, $funcsInModule, boolean($details))

What is happening here is somewhat beyond me.
And on top of all that when building exist from the develop branch you will get templating-1.2.0.xar in autodeplyoment.

@duncdrum
Copy link
Contributor

duncdrum commented Apr 10, 2025

And on top of all that when building exist from the develop branch you will get templating-1.2.0.xar in autodeplyoment.

On CI this is likely because there hasn't been a push event in the core repo since 1.2.1 was released that would have triggered a new container deploy.

@line-o
Copy link
Member Author

line-o commented Apr 10, 2025

Let me reiterate the failing tests are almost certainly unrelated and need to be fixed with #72
This PR can be merged before that.

@line-o line-o requested a review from duncdrum April 10, 2025 18:33
@duncdrum
Copy link
Contributor

@line-o i m not following. Without this change test on master are passing on ci, with it they fail.

Can you reproduce the test failure locally? What happens locally if you reverse the commit?

@line-o
Copy link
Member Author

line-o commented Apr 11, 2025

As I wrote in #72 when testing on my local machine if found the released version 1.2.1, master and this PR to be broken on exist 7 but all three run on exist 6

@duncdrum
Copy link
Contributor

duncdrum commented Apr 11, 2025

@line-o i agree that the typecast should not be necessary in theory, but practice shows it to be necessary. see above. This matches my local testing from when i did the housekeeping PR. Without the cast the app was broken, with it it worked.

I'll try to have a look at what happens on exist 7 by default on the weekend.

@line-o
Copy link
Member Author

line-o commented Apr 11, 2025

🤯 That's a worrying result
@duncdrum thanks for testing

@duncdrum
Copy link
Contributor

duncdrum commented Apr 11, 2025

yup the flight of the boolean bumblebee

@duncdrum duncdrum mentioned this pull request Apr 11, 2025
@line-o line-o closed this in #77 Apr 13, 2025
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.

3 participants