search

Interlisp / medley

The main repo for the Medley Interlisp project. Wiki, Issues are here. Other repositories include maiko (the VM implementation) and Interlisp.github.io (web site sources)
https://Interlisp.org
MIT License
376 stars 19 forks source link

Consider: re-save all medley/source files with current formatting for clean git diffs #1495

Open MattHeffron opened 10 months ago

MattHeffron commented 10 months ago

At least one recent pull request (#1487) showed extensive git differences, even though the actual changes were relatively localized. This seems to be due to PRETTYPRINT changing formatting, esp. differences in the "font change control characters". Also, some of the source files were saved with (MAKEFILE 'xxx 'FAST) and, if any of those files were edited, then the git differences would be (almost) the whole file, and pretty much useless.

My proposal would be to make a pass through the medley/source files and just do (LOAD 'xxx 'ALLPROP)(MAKEFILE 'xxx 'NEW) to get versions of the files with the current formatting. If this were in a single PR, then if it were approved and merged, then it should have git/GitHub perform new diffs, and they should be simpler.

Does this make sense? Is there a better way to accomplish this? What are the risks?

pamoroso commented 10 months ago

If I understand correctly, this makes a lot of sense. On GitHub this formatting results in walls of code which make it difficult to read not just diffs but the full sources.

rmkaplan commented 10 months ago

Is this when you do the diffs on git, or with the prc command inside Medley?

prc does the comparison ignoring white space (and I think also font shifts, but I don’t remember what I did there).

On Jan 10, 2024, at 10:38 AM, Matt Heffron @.***> wrote:

At least one recent pull request (#1487 https://github.com/Interlisp/medley/pull/1487) showed extensive git differences, even though the actual changes were relatively localized. This seems to be due to PRETTYPRINT changing formatting, esp. differences in the "font change control characters". Also, some of the source files were saved with (MAKEFILE 'xxx 'FAST) and, if any of those files were edited, then the git differences would be (almost) the whole file, and pretty much useless.

My proposal would be to make a pass through the medley/source files and just do (LOAD 'xxx 'ALLPROP)(MAKEFILE 'xxx 'NEW) to get versions of the files with the current formatting. If this were in a single PR, then if it were approved and merged, then it should have git/GitHub perform new diffs, and they should be simpler.

Does this make sense? Is there a better way to accomplish this? What are the risks?

— Reply to this email directly, view it on GitHub https://github.com/Interlisp/medley/issues/1495, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQSTUJOGM5VKLLMKCBA4EHLYN3N3RAVCNFSM6AAAAABBVIJKD2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGA3TIOJVGY3DENA. You are receiving this because you are subscribed to this thread.

MattHeffron commented 10 months ago

@rmkaplan You noted that GITFNS commands were having trouble with that PR due to the formatting changes, so it looks like it is a difficulty for GITFNS as well.

rmkaplan commented 10 months ago

Actually, my comment wasn’t that I thought that GITFNS had trouble with this one, I didn’t try it. My comment was that I thought that GITFNS would not have trouble with it.

On Jan 10, 2024, at 10:54 AM, Matt Heffron @.***> wrote:

@rmkaplan https://github.com/rmkaplan You noted that GITFNS commands were having trouble with that PR due to the formatting changes, so it looks like it is a difficulty for GITFNS as well.

— Reply to this email directly, view it on GitHub https://github.com/Interlisp/medley/issues/1495#issuecomment-1885437475, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQSTUJILSSHMFRJQVKPIH7TYN3PVPAVCNFSM6AAAAABBVIJKD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBVGQZTONBXGU. You are receiving this because you were mentioned.

pamoroso commented 10 months ago

To clarify, I mean formatting like this or this.

MattHeffron commented 10 months ago

@pamoroso Yes, that's the (MAKEFILE 'xxx 'FAST) format. (No pretty printing.)

nbriggs commented 10 months ago

GITFNS, in Medley, doesn't have any trouble with it. git diff and the web-based git diff do, of course, but I seldom try to use any git diff stuff on Lisp source code. It's marginally better if you tell it to ignore whitespace, but you can't tell it to ignore control characters. If you make any real changes the prettyprinting is quite likely to change even if you're comparing two prettyprinted versions, so the GITFNS code is still a better choice. I always end up using the branch-to-branch comparison, since I don't have gh installed.

masinter commented 10 months ago

There are risks that LOAD(X ALLPROP) MAKEFILE(X NEW) will not produce equivalent text. because of package, readtable or encoding problems. If you restarted a clean image for each file, the risks might be reduced. IT would be simple enough to load files, MAKEFILE NEW of them, and compare the READFILE of each.

There is a major risk that recompiling everything will trigger bugs, because of inconsistent declarations.

THe MEDLEY-UTILS file in the 'internal' directory has a few tests that detect a few of these.

nbriggs commented 10 months ago

Probably won't retain the upper/lower case depending on the readtables also.

MattHeffron commented 10 months ago

Ok, so using GITFNS is apparently the way to deal with differences of edited Medley files (e.g., in pull requests,). The next question is: What is the workflow using GITFNS? I have the local "clone" repositories and I created the corresponding "working-project" (empty) directories. If, in Medley, I edit a FNS (for example), it will load that definition 'PROP from the CLONEPATH. When I make changes, it will indicate that I must do MAKEFILE & COMPILE (or CLEANUP). But that will change the file in the CLONEPATH. What am I missing about using the WORKINGPATH so that the files in the CLONEPATH remain original? Or, do I have this backwards?

MattHeffron commented 10 months ago

In addition to git diffs, an additional potential value of reformatting the (MAKEFILE 'xxx 'FAST) files would be when doing any kind of text search in the sources, such as a "Find in files" type command in text editors (e.g., Notepad++), or Lispusers/GREP. Returning entire DEFUN, DEFPARAMETER, etc. as a single line for each hit makes the results very difficult to read.

rmkaplan commented 10 months ago

I may be the only one that uses the working directories, and use the gwc (git-working-compare) command to move things in or out of the currently checked out clone.

Others may work directly in their clone directory, and use prc (pull-request-compare) to compare against master. Or bbc (branch-to-branch-compare) to compare files in different branches (although I have recently seen some glitches with bbc, maybe needs to be updated in some way).

On Jan 12, 2024, at 11:10 AM, Matt Heffron @.***> wrote:

Ok, so using GITFNS is apparently the way to deal with differences of edited Medley files (e.g., in pull requests,). The next question is: What is the workflow using GITFNS? I have the local "clone" repositories and I created the corresponding "working-project" (empty) directories. If, in Medley, I edit a FNS (for example), it will load that definition 'PROP from the CLONEPATH. When I make changes, it will indicate that I must do MAKEFILE & COMPILE (or CLEANUP). But that will change the file in the CLONEPATH. What am I missing about using the WORKINGPATH so that the files in the CLONEPATH remain original? Or, do I have this backwards?

— Reply to this email directly, view it on GitHub https://github.com/Interlisp/medley/issues/1495#issuecomment-1889815573, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQSTUJK5654TYKEOCQP56NLYOGDCFAVCNFSM6AAAAABBVIJKD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBZHAYTKNJXGM. You are receiving this because you were mentioned.

masinter commented 10 months ago

There are a couple of possibilities, mutually exclusive:

Some considerations