Testament diff rework - unstructured, structured #206

haxscramper · 2022-01-26T19:13:24Z

wip, todo:

Changes for regular diff:

Convert this:

to this

and also make testament support structured reports that would allow you to get this

Out of this

discard """
nimoutFormat: sexp
cmd: "nim c --msgFormat=sexp --skipUserCfg --hints=on --hint=all:off --hint=User:on --filenames:canonical $file"
nimout: '''
(User :str "User Hint" :location ("tfile.nim" 8 _))
'''
"""

{.hint: "User hint".}

{.hint: "Another hint".} #[tt.Hint
      ^ (User :str "Another hint") ]#

Note how structured diff properly handles both inline and nimout error messages whereas with unstructured diff testament can only report half of the problems.

haxscramper · 2022-01-26T20:09:00Z

bors try

bors · 2022-01-26T20:11:00Z

try

Build failed:

All check passed

haxscramper · 2022-01-27T12:36:37Z

Sweep-grouping inline diffs is certainly necessary

that's what was generated before

saem · 2022-02-02T08:44:17Z

I tried to review, little too tired to digest this tonight, will try again tomorrow.

saem

Reviewed code, will do docs tomorrow 🤞

compiler/front/sexp_reporter.nim

saem · 2022-02-02T10:55:29Z

compiler/front/sexp_reporter.nim

+
+proc sexp*[E: enum](e: E): SexpNode = newSSymbol($e)
+
+proc sexpItems*[T](s: T): SexpNode =


Can we move this below the other s-exp procs, it flows better and if the T is one of the ones below it'll not be confusing if some declares a new one but this doesn't take it into account up here.

Also, seriously this probably shouldn't compile.

Also, seriously this process shouldn't compile.

what 'process' are you talking about and why it shouldn't compile?

Typo, yeah it's strange that it's allowed by the compiler.

it's strange that it's allowed by the compiler.

again, why?

Because a type T is not guaranteed to have an items, yes?

It won't compile when I instantiate the proc like every other generic, so I guess this is not allowed? I just don't get why this remark is attached to a definition. sexp two lines above can have $ defined as {.error.} and it would've been an instantiation error as well

saem · 2022-02-02T10:57:49Z

compiler/front/sexp_reporter.nim

+  sexp(writeConf.toMsgFilename(id))
+
+
+iterator sexpFields[T](obj: T, ignore: seq[string] = @[]): SexpNode =


So weird that this works without constraints, feels like a bug.

I think T should have an a type constraint of object | tuple

Why does it feel like a bug? Generic iterator, accepts generic type T, works as intended

I think T should have an a type constraint of object | tuple

Can be clarified later, but the fact it works without it doesn't really make it a bug IMO

It absolutely is a bug because there is no guarantee you can iterate over T?

What do you mean "can't iterator over T"? The iterator is defined for T therefore I can iterate over it, I don't see any bugs here

T is not guaranteed to have fields for fieldPairs to work, it might be out of scope for this PR, but it's silly that between the stdlib and the compiler that this is allowed.

You are talking about need for 'HasFields' concept or something like that here, otherwise it is a perfectly fine generic that would generate a perfectly fine compilation error on instantiation saying that there are no fields in here. I don't understand where the "bug" part comes from here. Just like the sexpItems above - it is instantiation error, just like every other generic proc. I can put object|tuple there, but that would just make code clearer, it won't make it more correct (as in - reject malformed program that would've compiled previously) in any way. If there is no field pairs the code does not compile, pointing to "instantiation of X...". If there are field pairs the code compiles.

compiler/front/sexp_reporter.nim

haxscramper · 2022-02-02T11:12:50Z

I will update the PR for review comments after you review the docs. Also, please do read the tests as well - I added some explanations about how diff is intended to function, and it's behavior there.

saem

Reviewed. There is plenty about the differencing I don't understand. So that'll be fun to dig into later.

Mostly typos and doc adjustments.

doc/nimc.rst

doc/testament.rst

testament/testament.nim

tests/stdlib/tcolordiff.nim

krux02 · 2022-02-03T17:22:30Z

what and why printing category ""?

haxscramper · 2022-02-03T17:43:44Z

I don't understand the question. The check for tests/**/t**.nim file was removed to allow testament run tlocalfile.nim - is that what you mean?

krux02 · 2022-02-03T20:06:22Z

I don't understand the question. The check for tests/**/t**.nim file was removed to allow testament run tlocalfile.nim - is that what you mean?

What I mean is the empty quotes in Test "tfile_regular.nim" in category ""

haxscramper · 2022-02-03T20:22:19Z

Fixed empty quotes, now it prints only test name for empty categories

@saem

- rework implementation of the message diff in testament. Now plaintext diff is implemented using built-in diff written in nim instead of calling into separate `git diff` command that produced noisy, colorless output and was completely unconfigurable. - Add implementation of the structured message diff, structured compiler reports for the compiler. These changes had to be made togehter, otherwise it would've been a series incomplete PRs that depended on each other even for the purposes of testing/documentation. - Add s-expression formatter to the compiler output. Selected using `--msgFormat=sexp` (for now we have a `text` and `sexp`, in the future `json` must be added as well, since that's what people actually expect from their tooling - not some obscure data format). - Fix 'invalid value' error for cli switch - that bug was discovered when adding `msgFormat` option. - Add structural S-expression diff algorithm with formatting logic for mismatches. - Add `nimoutFormat: sexp` field for the testament, to allow it understand what kind of data will be generated by the compiler itself. - Clean up testament message diff handling a little and add checks for structural diffs where appropriate. - Minor related changes - Split `strutils.addf` and reuse the interpolation string for colored text - otherwise all futher formatting logic looked a lot uglier than necessary (using `strformat.&` would require factoring out macro implementation parts) - Clean up testament documentation for different spec parts, add description of the structural messages. - More documentation all over testament implementation - Move `sexp` file into `experimental/` directory - it is no longer related directly to nimsuggest, @saem has already suggested moving it in nim-works#164 - Testament no longer has a hard assertion on the path of the tested file `testament tfile.nim` can also work, and does not force user to create needlessly nested directories for purposes of simple file testing.

saem

Bors r+

Merge me up, Scotty!

bors · 2022-02-06T12:18:34Z

Build succeeded:

haxscramper force-pushed the diff-everything branch from 43f7b54 to 431c4ea Compare January 26, 2022 19:14

haxscramper changed the title . Testament diff rework - unstructured, structured Jan 26, 2022

bors bot added a commit that referenced this pull request Jan 26, 2022

Try #206:

bec41a2

haxscramper closed this Jan 26, 2022

haxscramper reopened this Jan 26, 2022

haxscramper force-pushed the diff-everything branch from 558e0ee to b42b2c4 Compare January 26, 2022 20:48

ringabout mentioned this pull request Jan 27, 2022

misc std/editdistance timotheecour/Nim#397

Open

1 task

haxscramper force-pushed the diff-everything branch 10 times, most recently from cb4dbec to 2296000 Compare January 28, 2022 10:23

haxscramper requested a review from saem January 28, 2022 10:25

haxscramper mentioned this pull request Jan 28, 2022

Make testament actually understand the test specification #211

Open

saem requested changes Feb 2, 2022

View reviewed changes

saem requested changes Feb 3, 2022

View reviewed changes

haxscramper force-pushed the diff-everything branch from 2296000 to c4a2005 Compare February 3, 2022 16:41

haxscramper force-pushed the diff-everything branch from c4a2005 to 7252cbd Compare February 3, 2022 20:21

haxscramper force-pushed the diff-everything branch from 7252cbd to 8015d49 Compare February 6, 2022 08:56

saem approved these changes Feb 6, 2022

View reviewed changes

bors bot merged commit ce43c82 into nim-works:devel Feb 6, 2022

haxscramper added this to the Test suite reorganization and cleanup milestone Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testament diff rework - unstructured, structured #206

Testament diff rework - unstructured, structured #206

haxscramper commented Jan 26, 2022 •

edited

Loading

haxscramper commented Jan 26, 2022

bors bot commented Jan 26, 2022

haxscramper commented Jan 27, 2022 •

edited

Loading

saem commented Feb 2, 2022

saem left a comment

saem Feb 2, 2022 •

edited

Loading

haxscramper Feb 2, 2022 •

edited

Loading

saem Feb 2, 2022

haxscramper Feb 3, 2022

saem Feb 5, 2022

haxscramper Feb 5, 2022

saem Feb 2, 2022

haxscramper Feb 2, 2022

saem Feb 2, 2022 •

edited

Loading

haxscramper Feb 3, 2022

saem Feb 5, 2022

haxscramper Feb 5, 2022

haxscramper commented Feb 2, 2022

saem left a comment

krux02 commented Feb 3, 2022

haxscramper commented Feb 3, 2022 •

edited

Loading

krux02 commented Feb 3, 2022

haxscramper commented Feb 3, 2022

saem left a comment

bors bot commented Feb 6, 2022


		proc sexp*[E: enum](e: E): SexpNode = newSSymbol($e)

		proc sexpItems*[T](s: T): SexpNode =

		sexp(writeConf.toMsgFilename(id))


		iterator sexpFields[T](obj: T, ignore: seq[string] = @[]): SexpNode =

Testament diff rework - unstructured, structured #206

Testament diff rework - unstructured, structured #206

Conversation

haxscramper commented Jan 26, 2022 • edited Loading

haxscramper commented Jan 26, 2022

bors bot commented Jan 26, 2022

try

haxscramper commented Jan 27, 2022 • edited Loading

saem commented Feb 2, 2022

saem left a comment

Choose a reason for hiding this comment

saem Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

haxscramper Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saem Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haxscramper commented Feb 2, 2022

saem left a comment

Choose a reason for hiding this comment

krux02 commented Feb 3, 2022

haxscramper commented Feb 3, 2022 • edited Loading

krux02 commented Feb 3, 2022

haxscramper commented Feb 3, 2022

saem left a comment

Choose a reason for hiding this comment

bors bot commented Feb 6, 2022

haxscramper commented Jan 26, 2022 •

edited

Loading

haxscramper commented Jan 27, 2022 •

edited

Loading

saem Feb 2, 2022 •

edited

Loading

haxscramper Feb 2, 2022 •

edited

Loading

saem Feb 2, 2022 •

edited

Loading

haxscramper commented Feb 3, 2022 •

edited

Loading