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

fix: no error '! Couldn't find frame ID folio' anymore #1943

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jodros
Copy link
Contributor

@jodros jodros commented Dec 26, 2023

This "error" always annoyed me when I create framesets without a folio frame and I suppose other people do it was well.

What you think?

@jodros jodros requested a review from alerque as a code owner December 26, 2023 01:58
@jodros jodros marked this pull request as draft December 26, 2023 03:33
@jodros
Copy link
Contributor Author

jodros commented Dec 26, 2023

Well, even though it seemed to have worked sometimes, I noticed maybe it hasn't to do with this part lol, at least not at all, because it still give this error sometimes when using -d frames. But I just can't find where its declaration is in the code.

@jodros
Copy link
Contributor Author

jodros commented Dec 26, 2023

Now I finally got rid of it when running with -d frames.

The warning was pointing to where the endpage hook is declared, so I created a global variable in order to use the pcall check within the hook function.

BTW I'd like to know whether this is a good idea or not, and whether there are other ways to do this work without using global...

@jodros jodros marked this pull request as ready for review December 26, 2023 04:04
@Omikhleia
Copy link
Member

This "error" always annoyed me when I create framesets without a folio frame and I suppose other people do it was well.

It's perhaps a documentation problem? Silencing an error (absence of folio frame when the class expects it) is probably wrong, moreover using a global variable.

@jodros
Copy link
Contributor Author

jodros commented Dec 26, 2023

Silencing an error (absence of folio frame when the class expects it) is probably wrong.

My point it that the class shouldn't require a folio frame, when users create framesets without folio, they shouldn't be warned because of that. What is wrong in having no folio in the frameset?

@Omikhleia
Copy link
Member

What is wrong in having no folio in the frameset?

Nothing per se, but folios can be disabled with nofolios (or nofoliothispage), which can be invoked when switching to a frameset that doesn't need one anyway. And any "advanced enough" class would likely have sections with folios and sections without -- so it needs to do it properly anyway, and I am unsure of the benefit of hiding an error.

@jodros
Copy link
Contributor Author

jodros commented Dec 27, 2023

and whether there are other ways to do this work without using global...

🤦 It's impressive how simply getting enough sleep can make our brains function a bit better...

Now isFolioFrame is declared as local before the function.

@jodros
Copy link
Contributor Author

jodros commented Dec 27, 2023

I am unsure of the benefit of hiding an error.

I understand your point, but please note that the only "error" this PR would hide it not an actual error!

The current implementation of this package assumes every frame set as having a folio, what isn't true. It only would be an error unfairly hidden if the user wish to have a folio frame, but simple forgot to declare it, and received no warning, does this make any sense?

But the opposite can happen as happened with me, but why have to explicitly call nofolios when the package could just test whether there is the folio frame, and only call outputFolio() if so?

alerque

This comment was marked as duplicate.

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

I'm sorry, I do get what you are trying to "fix" here but something doesn't feel right. Like @Omikhleia has mentioned hiding an error just to hide an error doesn't seem right. Using pcall() to call external code that may or may not work right is one thing, but using it on our own internal code to spawn a bunch of work and just fail quietly if things go sideways and the work can't be done just isn't cosher. Sorry.

I do think there could be ways to fix this, but it is going to take a different approach. For now throwing an error if the user didn't turn off folios but did delete their expected frame is the right thing to do.

What about making folios on/off a class option and setting up the default frameset to not even create a folio frame if they are disabled?

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