search

JMSLab / Template

Template for research repository using scons.
9 stars 1 forks source link

Pull request for #68: Fix LyX handout #71

Closed marcopetterson closed 1 year ago

marcopetterson commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

Thanks @marcopetterson!

A couple things:

image

marcopetterson commented 1 year ago

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 commented 1 year ago

Thanks @marcopetterson, I looked over the code last night and I made some tweaks in https://github.com/JMSLab/Template/pull/71/commits/46d725f6c9899e78390eeac48fee037d8bbeab7d that more cleanly implement the solution outlined in https://github.com/JMSLab/Template/issues/68#issue-1400291808. 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 commented 1 year ago

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

ew487 commented 1 year ago

@rcalvo12 Just to make sure, are we still going with the naming conventions in https://github.com/JMSLab/Template/issues/68#issuecomment-1270768511?

jmshapir commented 1 year ago

@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 https://github.com/JMSLab/Template/issues/68#issue-1400291808 not self-contained?

ew487 commented 1 year ago

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 commented 1 year ago

@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 https://github.com/JMSLab/Template/pull/71/commits/60595be1e2289555e51a22ca5d74fa153dbaf5c3 I went ahead and updated the naming conventions.

ew487 commented 1 year ago

Yes @ew487, apologies I missed that comment so that was not implemented. In https://github.com/JMSLab/Template/commit/60595be1e2289555e51a22ca5d74fa153dbaf5c3 I went ahead and updated the naming conventions.

@rcalvo12 Got it, thanks very much!

marcopetterson commented 1 year ago

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 https://github.com/JMSLab/Template/commit/60595be1e2289555e51a22ca5d74fa153dbaf5c3.

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

jmshapir commented 1 year ago

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 https://github.com/JMSLab/Template/issues/68#issue-1400291808 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 commented 1 year ago

@marcopetterson @jmshapir, yesterday me and @ew487 worked together on this issue and we found a solution (pushed in https://github.com/JMSLab/Template/pull/71/commits/8a6eb9c2218f63a283e881082c0450f8fcc3919a) that both allows the tests to pass and seems to maintain the functionality we are hoping for.

jmshapir commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.