move icon indicating copy to clipboard operation
move copied to clipboard

[WIP][move-docgen] Making Docgen independent of Prover

Open villesundell opened this issue 2 years ago • 7 comments

Docgen was originally developed as a part of the Prover. However, Docgen is now independent of Prover, so moving it from Prover to its own directory seems the next logical step.

Motivation

Docgen was buried inside Prover for no apparent reason (now).

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Unit tests pass: moved Docgen unit tests, tests for move docgen, and all the previous tests.

villesundell avatar Aug 23 '22 18:08 villesundell

Pinging my mentors @awelc and @tnowacki for review :memo:

villesundell avatar Aug 23 '22 18:08 villesundell

@wrwg Sorry, I am not following here: if I understand correctly Docgen is nowadays free from Prover dependencies (docgen.rs) and can be now (#327) used directly with move docgen without calling/using Prover?

I even experimented removing the last dev-dependencies in this commit and move docgen compiles and works correctly. However test is still using Prover, and that's something I have to change, thank you for the pointer :+1:

It's true that Prover is still using Docgen, and hence is depending on it, but to me that seems like another tasks to remove Prover's dependence on Docgen. So to me it looks like Docgen is independent of Prover (and hence needs to be moved), and Prover is not independent of Docgen (but must be in the future).

But changing this to WIP at least because of the test refactoring (and for possible refactoring in general).

villesundell avatar Aug 24 '22 13:08 villesundell

@wrwg Sorry, I am not following here: if I understand correctly Docgen is nowadays free from Prover dependencies (docgen.rs) and can be now (#327) used directly with move docgen without calling/using Prover?

I even experimented removing the last dev-dependencies in this commit and move docgen compiles and works correctly. However test is still using Prover, and that's something I have to change, thank you for the pointer 👍

It's true that Prover is still using Docgen, and hence is depending on it, but to me that seems like another tasks to remove Prover's dependence on Docgen. So to me it looks like Docgen is independent of Prover (and hence needs to be moved), and Prover is not independent of Docgen (but must be in the future).

But changing this to WIP at least because of the test refactoring (and for possible refactoring in general).

The prover has never been using docgen. Instead the only way to call docgen was over the prover driver (this is the same for some other tools). It is great that you point out that you made now a direct entrypoint via the Move CLI. Still can you please cut off the remaining, now unnecessary dependency? It's the one and only one AFAICT. If the docgen tests are still depending on the entry over the prover, it shouldn't be too hard to fix this as well. I'm a bit concerned about saying "next PR", because those things can be easily forgotten. It's a similar concern I had with the docgen options, where we have now some kind of duplication.

BTW for the background, docgen is a tool I've written, so I've some stake here ;-)

Thanks for trying to clean this up.

wrwg avatar Aug 24 '22 15:08 wrwg

Ah and one additional remark. We where actually trying to locate all tools like docgen in the tools folder. Tools which are currently outside are legacy. Perhaps you can move docgen into that sub-directory. Thank you.

wrwg avatar Aug 24 '22 15:08 wrwg

@wrwg Ah, okay! So, I see these two steps next for me (probably one commit each):

  • Remove Prover dependency in test
  • Remove Docgen code from Prover

(Current Docgen does not seem to be stand-alone, so probably it's not mature enough for tools just yet?)

villesundell avatar Aug 24 '22 18:08 villesundell

(Current Docgen does not seem to be stand-alone, so probably it's not mature enough for tools just yet?)

Not sure what you mean -- definitely, if it would not be mature, it would even more belong into tools instead of top-level. In general, we do not have such categories influencing the location of a crate. But there was a decision made to move all tools into the tools directory a while ago.

wrwg avatar Aug 24 '22 23:08 wrwg

@wrwg Ah, sorry: at quick glance it seemed to me that tools had only the tools which are stand-alone, usable directly from the command line. But now I see that there are some (like move-package) which are not stand-alone. So yeah, I agree, maybe tools is the best place for move-docgen :+1:

villesundell avatar Aug 25 '22 15:08 villesundell