Fix #3349: Fix dottydoc filenames for Windows #4171
Conversation
smarter
force-pushed
the
fix/dottydoc-windows
branch
from
5f4b5f0 to
ce30255
Compare
| def path(sym: Symbol)(implicit ctx: Context): List[String] = { | ||
| @tailrec def go(sym: Symbol, acc: List[String]): List[String] = | ||
| if (sym.isRoot) | ||
| acc |
There was a problem hiding this comment.
I think this should be acc.reverse, unless you meant to change the semantics unlike described (the caller doesn't call reverse either).
This appears return a reverted list compared to earlier — since you essentially replaced xs :+ x by x :: xs.
There was a problem hiding this comment.
no, the semantics are the same as before, a.b.c becomes List("a", "b", "c").
There was a problem hiding this comment.
Sorry, you're right, brain short-circuit.
| //TODO: should not `collectMember` from `rhs` - instead: get from symbol, will get inherited members as well | ||
| ObjectImpl(o.symbol, annotations(o.symbol), name.dropRight(1), collectMembers(rhs), flags(o), path(o.symbol).init :+ name, superTypes(o)) | ||
| ObjectImpl(o.symbol, annotations(o.symbol), o.name.stripModuleClassSuffix.show, collectMembers(rhs), flags(o), path(o.symbol.owner) :+ o.name.mangledString, superTypes(o)) |
There was a problem hiding this comment.
Is o.name == o.symbol.name (I tried to find out but failed)? Then path(o.symbol) would be simpler and faster. Otherwise, maybe just pass o.name.mangledString :: Nil as initial accumulator to avoid the O(N) append.
I'd also factor out map (_.mangledString) for robustness.
Many characters are forbidden in Windows filenames, mangled names are used for classfiles so they should be safe. Also make factories#path more efficient.
smarter
force-pushed
the
fix/dottydoc-windows
branch
from
ce30255 to
54ccac6
Compare
| //TODO: should not `collectMember` from `rhs` - instead: get from symbol, will get inherited members as well | ||
| ObjectImpl(o.symbol, annotations(o.symbol), name.dropRight(1), collectMembers(rhs), flags(o), path(o.symbol).init :+ name, superTypes(o)) | ||
| ObjectImpl(o.symbol, annotations(o.symbol), o.name.stripModuleClassSuffix.show, collectMembers(rhs), flags(o), path(o.symbol), superTypes(o)) |
There was a problem hiding this comment.
I would do o.name.stripModuleClassSuffix.toString. show is for pretty printing, it might add colors
There was a problem hiding this comment.
I don't think show adds color on names, but if we change this then there's a ton of places where this is used in dottydoc that need to be changed. I'll leave that to whoever wants to maintain dottydoc :).
Many characters are forbidden in Windows filenames, mangled names are
used for classfiles so they should be safe.
Also make factories#path more efficient.