Pull request for #68: Fix LyX handout

[marcopetterson]

@rcalvo12 or @ew487 could you take a look at this?

After studying the unit test guide and trying to understand what the existing ones do I think we do not need to add new ones or modify existing ones. Please let me know if you disagree

Of course @jmshapir prefers somebody else takes a look I am all for it

Thank you!

[marcopetterson]

After having opened this PR I see some checks were not successful, will take a look at why but open to suggestions from someone who knows ubuntu.

Thank you!

[jmshapir]

Thanks @marcopetterson!

A couple things:

• Since @rcalvo12 opened the issue originally I am assigning @rcalvo12 as reviewer but we can switch if needed
• @marcopetterson have you been able to run test_build_lyx locally? If not I would try that first. Based on the test summary (screenshot below) I am guessing there is an error that is not related to the OS.

[marcopetterson]

Thanks @jmshapir!

I had not tried running test_build_lyx locally and indeed I get the same error.

I believe it is related to the fact that the source file it is looking for is actually an intermediate file that is created in the process of creating the handout. Currently trying to finding a fix, either in the way that the file is created or in the unit test

[rcalvo12]

Thanks @marcopetterson, I looked over the code last night and I made some tweaks in 46d725f that more cleanly implement the solution outlined in #68 (comment). I also could not get the tests to pass locally after much struggle. @ew487 when you have time could you look at what is going wrong here, since I believe you wrote the tests for this?

[ew487]

@jmshapir Could I please get access to PolEc #7 so I can see what is going on?

[ew487]

@rcalvo12 Just to make sure, are we still going with the naming conventions in #68 (comment)?

[jmshapir]

@ew487 thanks!

@rcalvo12 Just to make sure, are we still going with the naming conventions in #68 (comment)?

I support those conventions and I don't think we've heard any objections, so I vote yes!

@ew487 have these conventions been implemented? If not, would they be difficult to implement? If not difficult, do you think you could implement as part of your review here?

@jmshapir Could I please get access to PolEc #7 so I can see what is going on?

@ew487 happy to give you access but the issue in that other repository is long since resolved. Is the description in #68 (comment) not self-contained?

[ew487]

I support those conventions and I don't think we've heard any objections, so I vote yes!

@ew487 have these conventions been implemented? If not, would they be difficult to implement? If not difficult, do you think you could implement as part of your review here?

@jmshapir I don't think they have been implemented, and I think it should be straightforward to do so I'll give it a try!

@ew487 happy to give you access but the issue in that other repository is long since resolved. Is the description in #68 (comment) not self-contained?

Oh, I see! I wanted to check that the stuff I implement will in fact fix the problem. Is the problem already exhibiting in the current version on master?

[rcalvo12]

@rcalvo12 Just to make sure, are we still going with the naming conventions in #68 (comment)?

Yes @ew487, apologies I missed that comment so that was not implemented. In 60595be I went ahead and updated the naming conventions.

[ew487]

Yes @ew487, apologies I missed that comment so that was not implemented. In 60595be I went ahead and updated the naming conventions.

@rcalvo12 Got it, thanks very much!

[marcopetterson]

Thank you @rcalvo12! I implemented it by adding the *.handout.* as per #68(comment) but I see now we are back to the same naming conventions as per 60595be.

I have also tried to improve on the tests running them locally during this time but so far with no success

[jmshapir]

Oh, I see! I wanted to check that the stuff I implement will in fact fix the problem. Is the problem already exhibiting in the current version on master?

@ew487 thanks! The answer is yes and no. :-) The reason is that the problem arises when the handout option is called and the file locations have a particular structure. The version of the Template in main doesn't have such a structure so the handout builds fine. But from #68 (comment) I gather it should be pretty straightforward to reproduce the error in a branch off of main by, say, adding an input file sourced from a relative path. If it turns out not to be straightforward to I am guessing @rcalvo12 can help, and if neither of you can reproduce it then maybe there was no problem in the first place! :-)

[rcalvo12]

@marcopetterson @jmshapir, yesterday me and @ew487 worked together on this issue and we found a solution (pushed in 8a6eb9c) that both allows the tests to pass and seems to maintain the functionality we are hoping for.

[jmshapir]

OK!

@marcopetterson can you let us know if the revision looks good to you?

If so I think we are all set here thanks @rcalvo12 @ew487!

[rcalvo12]

Just confirming that I have tested a situation very similar to the one that originally gave us problems in PolEc, and I am happy to report that this solution does seem to fix the problem. I think we should be good to go!

[marcopetterson]

Thank you @jmshapir @rcalvo12 and @ew487!

The revisions look good to me and I have also tested a few behavior similar to what created issues as described in #68(comment). If everyone agrees then I will go ahead and squash and merge this into the main branch

[jmshapir]

Thank you @jmshapir @rcalvo12 and @ew487!

The revisions look good to me and I have also tested a few behavior similar to what created issues as described in #68(comment). If everyone agrees then I will go ahead and squash and merge this into the main branch

@marcopetterson thanks and sounds good! I think you can go ahead and merge when ready.