Conversation
…suites. Added some documentation.
olafurpg
left a comment
olafurpg
left a comment
There was a problem hiding this comment.
Thank you for this contribution! I just did a quickly review, will need a closer look another time
| import java.nio.file.Paths | ||
|
|
||
| import mdoc._ | ||
| import mdoc.internal.cli.{Exit, Settings} |
There was a problem hiding this comment.
Settings can’t be exposed in the public api since it’s internal. We could add a public interface on the class if that helps
There was a problem hiding this comment.
@olafurpg Apologies for closing this but the CI/CD failed. In particular one tests assumes that the files are processed in a given order. Need to correct this.
Settings are not required so I will remove it.
There was a problem hiding this comment.
You can add MainSettings as an argument
We can add public methods down the road to access fields on the private Settings
There was a problem hiding this comment.
Alternatively, we can add a abstract class Settings in the public mdoc API and pass that in even if it has no public members for now. It's desirable to pass in one parameter just so we can pass in more information in the future without breaking the public API
There was a problem hiding this comment.
I don't understand the use of MainSettings. Settings does not extend this class. Note that I call onStart method in MainOps. Can you explain?
There was a problem hiding this comment.
Settings is a case class which gets auto generated methods like “copy” that break binary compatibility on removal/addition of fields. This makes case classes essentially unsuitable for public APIs unless you’re certain you will never need to add/remove a field.
MainSettings is just a thin wrapper around the case class that has a stable api that we can evolve in a binary compatible way.
There was a problem hiding this comment.
Thanks for the explanation. MainSettingsis now being used. I have tried to make as few changes as possible. They are basically limited to MainOps. Not too happy with it because I would have liked to make the calls within the MainOps.runmethod. However this would entail changing the parameters of the constructor or run (Context is used in many places and my not be the best change).
Please see #245
|
|
||
| trait PostModifier { | ||
| val name: String | ||
| def onStart(settings: Settings): Unit |
There was a problem hiding this comment.
These need to have default implementations for backwards compatibility
There was a problem hiding this comment.
Ok. Will do. Will a simple return of () do?
Apologies for the newbie questions.
I as planning to make another pull request. Should I or do I keep working here? If I do open a new one, their seem to be a "review pull " option. Should I use that instead?
There was a problem hiding this comment.
: Unit = () is a fine default implementation
2 participants
Pull request for #213. Note that
onWatchIterationEndnot implemented.