Implement #2 #13
Implement #2 #13
Conversation
| var depContents = _yamlMap['dependencies'] != null ? _block('dependencies') : ''; | ||
| var devdepContents = _yamlMap['dev_dependencies'] != null ? _block('dev_dependencies') : '';; | ||
|
|
||
| return "$fieldContents$envContents$depContents$devdepContents"; |
There was a problem hiding this comment.
I don't think we can assume this order or that there are not other fields to include. I think it needs to go through the contents of the _yamlMap and generically highlight things. I'd be ok with saving the semantic highlighting for later though.
|
@seaneagan In a later PR, I would like to then have a property in Pubspec for syntax highlighted contents. Thank you, again. |
| return new Question("$field (${_yamlMap[field]})"); | ||
| }); | ||
|
|
||
| @SubCommand(help: 'Initiate or edit pubspec.yaml file.') |
There was a problem hiding this comment.
"pubspec.yaml file" -> "a pubspec"
|
That's all I have for now. It's getting close! |
|
@seaneagan Thank you for your patience and help! Will keep pushing to make it right! |
|
@seaneagan , finished edits. Should we add logging to the project and log errors? Also, since we made Pubspec.init async, should we also make Pubpsec.load async as well? I also want to try building tests for each command. Any of these you like, I can create separate issues for tracking. Thank you again for walking me through this. I learned so much and open to more edits/suggestions as well. |
| @@ -87,6 +108,41 @@ class Pubspec { | |||
| this._contents, | |||
| this._yamlMap); | |||
|
|
|||
| static Future<Pubspec> init() { | |||
| var completer = new Completer(); | |||
There was a problem hiding this comment.
I always implement Future-returning methods with something like:
=> new Future(() { /* code */ })
to ensure all errors are caught, and for readability (clear that a Future is actually returned).
There was a problem hiding this comment.
Most of the time Completers can be avoided, they shouldn't be required here.
|
@damondouglas Hey no problem at all, thanks for working on this! Done reviewing this round. Tracking issues for logging, command tests, and async Pubspec.load sound good. |
|
@seaneagan , Here is this round of edits. I tried to have an elegant parser map but kept having a |
|
@damondouglas this looks like a good start, merging. Thanks for working on this! There are still a few things that don't work from my testing, so I'm going to fix those shortly. Then I'm going to fix #5 and attempt to integrate den into stagehand before releasing the |
|
@seaneagan , thank you for kindly letting me try this and for your help. I'm going to start working on #15. |
@seaneagan thank you so much for this opportunity. Even if my PR is not accepted, I appreciate what I've learned from your code and this experience.