Faré wrote:
Dear Robert,
here are some code review comments on ASDF 3.0.2.1.
Thank you very much for these comments. They are hugely helpful, and I will act on them.
- Putting :asdf/find-system after :asdf in the recycle looks like it's
defeating the purpose; you might as well remove it.
Will do. I didn't get around to documenting RECYCLE per our conversation on the ticket. I have pushed a revision (3.0.2.2) with documentation, perhaps you could review? In particular, I don't adequately understand the :MIX option, so have left it without documentation.
Would it be reasonable to have DEFINE-PACKAGE have keyword arguments as well as the &rest parameter? The idea would be to make the information an emacs (or other) environment displays to the programmer be more useful.
Another related question: in ENSURE-PACKAGE, there's a case where we ignore names that the programmer has asked us to UNINTERN (when they are :INHERITED). Question: should we be signaling this with a WARNING, in case the user's expressed intent is being violated? Or is this something that must be violated sometimes in order to effectively upgrade (in which case we should leave this here).
- Maybe refactor duplicate-names so it doesn't inherit from
system-definition-error ? Or have a function of the same name be called that is defined later to throw the error?
That seems possible, or perhaps we should just have an ASDF/CONDITIONS package and kick *all* of the conditions up the dependency order? If you want one, just import it or use this package. That seems possibly to cause upgrade issues, though, so I have left this undone.
- Since we don't work on antique cmucl, maybe we could check whether
the :report print-object hack is still needed?
I haven't been testing on either cmucl or abcl, it seems. I will restore those, and maybe we could do this.
Raymond seems like the authority on this.
- I kind of wanted to prevent the multiplication of .asd files and the
eating away of the namespace by using test-asdf/foo systems for new systems; but it's your baby, so your call. Also, an error might not be compatible with putting it in test-asdf.asd. On the other hand, that's a case where define-test-system and/or eval can be used so you don't need a .asd file at all.
Thank you; I will think about making these changes.
- the nodes in the grammar could be renamed to be less confusing and
not require a comment.
Agreed. Also, I'm not sure about the syntax that's used here. There are some uses of @var markup, but it's not clear to me what that signifies. I suspect we haven't used it consistently as the texinfo has accumulated over time.
The manual really needs an end-to-end overhaul, but I don't see myself as having the time to do this in the near future.
best,
r
—♯ƒ • François-René ÐVB Rideau •Reflection&Cybernethics• http://fare.tunes.org The politicians have a most touching faith in technology — that it can make up for any dumb thing the politicians decide to do. — John McCarthy