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

Remove unnecessary the and remove GamePhysics from table of abbrv. #3528

Merged
merged 20 commits into from
Jul 25, 2023

Conversation

janim2-2004
Copy link
Collaborator

@janim2-2004 janim2-2004 commented Jul 11, 2023

closes #3514
closes #3491
closes #3521

code/drasil-example/nopcm/lib/Drasil/NoPCM/Body.hs Outdated Show resolved Hide resolved
code/stable/dblpendulum/SRS/HTML/DblPendulum_SRS.html Outdated Show resolved Hide resolved
code/stable/nopcm/SRS/HTML/NoPCM_SRS.html Outdated Show resolved Hide resolved
code/datafiles/nopcm/SystemContextFigure.png Outdated Show resolved Hide resolved
code/stable/nopcm/SRS/HTML/NoPCM_SRS.html Outdated Show resolved Hide resolved
@janim2-2004
Copy link
Collaborator Author

@samm82 using the abbreviation DblPend leads to issues with the tex file and code generation. To resolve this we would need to change the name of the example to DblPend, is that ok to do? (This also applies to SglPend)

@samm82
Copy link
Collaborator

samm82 commented Jul 15, 2023

Can you be more specific? I'm not sure what the issue is. Why would changing the name of the example to "DblPend" fix it? Because our file structure is set up to use the abbreviations of examples for their names when generating folders, etc.?

@janim2-2004
Copy link
Collaborator Author

janim2-2004 commented Jul 17, 2023

Because our file structure is set up to use the abbreviations of examples for their names when generating folders, etc.?

This is exactly why. The pdf document, being the only one automated for a name change, tries to find the directory "dblPend" and and it is not able to find it. to get it to work we would only need to change the generated folder names but it is probably better to change the whole example name to keep it consistent.

Alternatively this may just be an abuse of the abbreviation and we could also just remove this automation from the tex generation, but that seems like a step backwards.

I think there is a bigger issue here where we should be able to just change the name of an example but that is beyond the scope of this PR, let me know if there should be an issue about this.

@samm82
Copy link
Collaborator

samm82 commented Jul 17, 2023

Changing the whole example name seems like the best move here, and is probably OK to be done in this PR (even though it is getting a bit big, the work seems to be related, except for maybe the work done for #3491)

@smiths
Copy link
Collaborator

smiths commented Jul 17, 2023

I'm okay with changing the name of the example. The default of having the program name and its abbreviation being the same makes sense. I do think that we might eventually wish to allow for the possibility of the name and the abbreviation being different, but this isn't a priority.

@JacquesCarette
Copy link
Owner

Changing the name of the examples seems to be the most coherent solution with respect to the discussion. (I'll reply to other items separately).

@janim2-2004 janim2-2004 marked this pull request as draft July 18, 2023 14:24
@janim2-2004 janim2-2004 marked this pull request as ready for review July 19, 2023 17:57
@janim2-2004 janim2-2004 requested a review from samm82 July 19, 2023 18:09
@janim2-2004
Copy link
Collaborator Author

@samm82 I am not sure why gool test is breaking I have taken a look at it but I am not able to find any reason why. The issue seems to only be in SWHSNoPCM with the other 2 examples that had their names changed working fine. If you could take a look, I would appreciate it as I am a little lost.

@hrzhuang
Copy link
Collaborator

hrzhuang commented Jul 20, 2023

The reason make gool was breaking is that it relies on symlinks such as datafiles/dblpend/cpp (which points to datafiles/odelib/cpp) in order to find the odelib. But these are UNIX symlinks, which are not recognized as symlinks on @ManvendraJani's computer, so when dblpendulum was renamed to dblpend, the symlinks turned into plain text files (containing the text ../odelib/cpp, for example). I just pushed a fix for this.

This uncovers a bigger issue of UNIX symlinks in our repository. As it currently stands (even before the changes in this PR), make gool does not work on Windows under Git Bash because the symlinks do not work (and we haven't realized this because make gool usually just runs on the CI 😅). I'm not sure if it would work under Cygwin, but in any case I don't think we should be relying on UNIX symlinks in a repository that is supposed to be cross-platform. I have created issue #3549 for this.

@samm82
Copy link
Collaborator

samm82 commented Jul 20, 2023

Sorry I didn't get a chance to look at this earlier, but good job for figuring it out! 🎉

Copy link
Collaborator

@samm82 samm82 left a comment

Choose a reason for hiding this comment

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

This looks good to me! (Still confused as to why the resolution of the SWHS figure dropped, but it seems like it is of high-enough quality so I don't know how big of a deal it is ¯\_(ツ)_/¯)

Copy link
Owner

@JacquesCarette JacquesCarette left a comment

Choose a reason for hiding this comment

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

Just one question before I approve.

code/datafiles/swhs/SystemContextFigure.png Outdated Show resolved Hide resolved
Copy link
Owner

@JacquesCarette JacquesCarette left a comment

Choose a reason for hiding this comment

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

Approving now, but I'm going to wait a bit to see if others have comments, as this has been quite the storied PR!

@smiths
Copy link
Collaborator

smiths commented Jul 20, 2023

Given the comment from @JacquesCarette I reviewed again. I commented in stable the parts that don't seem quite right to me. Mainly the comments are around DblPend and SglPend. It seems odd that we give the programs these names, but we also use the names Double Pendulum and Single Pendulum. I guess we have the long-form version of the name and the abbreviation. I'd be okay if we just called them by the short forms, or if we officially gave them the name of the long forms, but explicitly introduce that we will use the short form going forward (like we do with SWHS).

@smiths smiths self-requested a review July 21, 2023 14:50
Copy link
Collaborator

@smiths smiths left a comment

Choose a reason for hiding this comment

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

This PR looks good to me with respect to the changes to stable.

@JacquesCarette
Copy link
Owner

Sorry, I made more merge conflicts when I pulled in another PR...

@JacquesCarette JacquesCarette merged commit 8d5e8f8 into master Jul 25, 2023
5 checks passed
@JacquesCarette JacquesCarette deleted the remove-unnecessary-the branch July 25, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants