Add support for declarations in action config files #1630
Conversation
core/actions/declaration.ts
Outdated
| constructor(session?: Session) { | ||
| constructor(session?: Session, config?: dataform.ActionConfig) { | ||
| super(session); | ||
| if (!session || !config) { |
There was a problem hiding this comment.
I see why you do this - a slightly cleaner / less error prone way would be to overload the constructor to have a 0 params form and a 2 params form where they are both required, e.g: https://stackoverflow.com/questions/12702548/constructor-overload-in-typescript
There was a problem hiding this comment.
This doesn't seem to work the way one would expect it to with type overloading.
How about now, where just the presence of config is checked?
| declaration.proto.fileName = utils.getCallerFile(this.rootDir); | ||
| public declare(declarationConfig: dataform.ITarget | dataform.ActionConfig): Declaration { | ||
| // The declaration property exists on ActionConfig but not on Target. | ||
| if (!declarationConfig.hasOwnProperty("declaration")) { |
There was a problem hiding this comment.
This is pretty nasty! Two paths here and two paths in the constructor...
Surely there must be a way to push this all into the constructor at least?
There was a problem hiding this comment.
This has to be nasty somewhere in order to preserve the previous functionality - one alternative would be to just stop supporting Target as a parameter.
I'm not keen for it being in the constructor, because I think it obfuscates that it is the method on session that contains deprecated functionality, as opposed to the class constructor which isn't something the user can call directly.
There was a problem hiding this comment.
I still think there probably is some way to make this a little nicer, but I see you have a pattern here so let's stick to that until we see a better way :)
| declaration.proto.fileName = utils.getCallerFile(this.rootDir); | ||
| public declare(declarationConfig: dataform.ITarget | dataform.ActionConfig): Declaration { | ||
| // The declaration property exists on ActionConfig but not on Target. | ||
| if (!declarationConfig.hasOwnProperty("declaration")) { |
There was a problem hiding this comment.
I still think there probably is some way to make this a little nicer, but I see you have a pattern here so let's stick to that until we see a better way :)
* Add support for declarations via action config files * Tidy * Fix lint * Tidy constructors
No description provided.