On 4/10/20 8:51 AM, Hannes Wallnoefer wrote:
With this changeset there are now three get*ListItem(Content) methods in
SubWriterHolderWriter, all with the same implementation returning
HtmlTree.LI(content). These methods are only called from the local
`formats.html` package. Two of them, #getSummariesListItem and
#getDetailsListItem are also defined in the ClassWriter interface, although
they are never invoked on that interface or the implementing ClassWriterImpl
class, only on SubWriterHolderWriter.
I'll investigate the two methods that I've put on ClassWriter that are
never called. Admittedly SubWriterHolderWriter is itself a bit of a
code-smell, and one I have not yet figured out how best to fix; the
unused methods may be a side-effect of that.
I guess all of the above could be justified by completeness and symmetry (for
lack of better terms). But I wonder if these methods actually do carry their
weight, both for having distinct methods and for defining some of them in
ClassWriter.
Note that this is not a criticism, just something I’m curious about.
It's a valid comment, and something I have been considering as well. I
partially touched on this in some of earlier RFR notes.
At some level, it's the pendulum swinging to the opposite extreme from
the poorly-named overused getMemberTree-style methods that used
blockList for everything.
There are various ways forward, preferably in separate changesets.
1. Leave as-is, with multiple like-named methods, with the same impl.
This gives us the flexibility we need, if we ever want it, to set a
different `class` attribute on different impls.
2. Collapse the `getXyzList` and `getXyzListItem` methods into just two,
`getList` and `getListItem`, where the `getList` method and possibly the
`getListItem` methods take a parameter for a new enum type, `ListKind`.
3. Have the `getXyzList` methods return a different type
(`ListContent`?) such that `add`ing a method to these objects will
automatically wrap the item in a suitable container, which may be either
a simple `<li>` element, or an `<li>` element with an appropriate class
attribute.
3a. Like 3, but without an explicit new type of Content ... just a new
impl behind the scenes, such as special subtype of HtmlTree for UL/OL
elements.
3b. Some combination of 1, 2 and 3a. Either use separate `getXyzList`
methods or `getList(ListKind)`, but collapse the `getXyzListItem`
methods into one, and leave it up to the list object to decide whether
to "customize" individual list items returned from the general
getListItem method.
Whether to put a `class` attribute on a `<li>` element depends on
whether we want, or may want in the future, to use BEM naming for our
CSS classes. In full BEM, every HTML element gets its own `class`
attribute with an appropriate name. Right now, my sense is to /not/ use
full BEM naming when there is an inherent HTML relationship, such that
we can use the `>` selector, so that we can style the list items with
`ul.LIST-CLASS > li { STYLES }`
---
For now, I'd like to leave this as an open/active discussion. Although
there are more uses of `<ul class="block-list">` on other pages, we have
addressed the 3 primary uses on the type-declaration pages. I'd like to
continue the cleanup for other parts of module/package/type declaration
pages, and see what other cleanup we would like to do. For example, I'd
like to consider a impl-method naming cleanup to use `newXyz` or
`buildXyz` instead of `getXyz` ... I'm thinking `newXyz` for basic
factory methods, and `buildXyz` for more complex constructions. I'd also
like to move away from the `addXyz(Content container)` style methods in
which we build something and add it to a container in a single call, and
move towards `container.add(buildXyz())` as providing a better
separation of abstractions.
-- Jon
