[NPMCLI-53] Adding availability removes comments and empty lines from existing yaml Created: 24/Oct/16 Updated: 15/Mar/17 Resolved: 08/Nov/16 |
|
| Status: | Closed |
| Project: | Magnolia CLI |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 1.0 |
| Type: | Bug | Priority: | Major |
| Reporter: | Tomáš Gregovský | Assignee: | Federico Grilli |
| Resolution: | Fixed | Votes: | 1 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||
| Template: |
|
||||||||||||||||||||
| Acceptance criteria: |
Empty
|
||||||||||||||||||||
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||||||||||||||
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
||||||||||||||||||||
| Date of First Response: | |||||||||||||||||||||
| Sprint: | Basel 69 | ||||||||||||||||||||
| Story Points: | 3 | ||||||||||||||||||||
| Description |
|
If you have customized page definition, e.g added comments, including something, or just using empty lines to make code easy to read. but when creating new component which has parameter for availability which at the end modifies customized page definition, all that fancy stuff (named above) are removed... |
| Comments |
| Comment by Christopher Zimmermann [ 03/Nov/16 ] |
|
OK - this is very bad because it is destroying work, especially since the user does not know and might discover that only later. |
| Comment by Federico Grilli [ 04/Nov/16 ] |
|
Had a look at the issue and, as far as I could see, only comments and empty lines are removed, but luckily NOT new elements. As bad as this can be, perhaps the issue is not a blocker. UPDATE
mapping values are not allowed here
on line 3, column 10
at ScannerError.YAMLError [as constructor] (node_modules/yaml-js/lib/errors.js:70:46)
at ScannerError.MarkedYAMLError [as constructor] (node_modules/yaml-js/lib/errors.js:90:45)
at new ScannerError (node_modules/yaml-js/lib/scanner.js:23:49)
at Constructor.__dirname.Scanner.Scanner.fetch_value (node_modules/yaml-js/lib/scanner.js:504:19)
at Constructor.__dirname.Scanner.Scanner.fetch_more_tokens (node_modules/yaml-js/lib/scanner.js:214:21)
at Constructor.__dirname.Scanner.Scanner.peek_token (node_modules/yaml-js/lib/scanner.js:138:14)
at Constructor.__dirname.Parser.Parser.parse_document_end (node_modules/yaml-js/lib/parser.js:180:20)
at Constructor.__dirname.Parser.Parser.get_event (node_modules/yaml-js/lib/parser.js:98:46)
at Constructor.__dirname.Composer.Composer.compose_document (node_modules/yaml-js/lib/composer.js:117:12)
at Constructor.__dirname.Composer.Composer.get_single_node (node_modules/yaml-js/lib/composer.js:91:25)
at Object.__dirname.compose (node_modules/yaml-js/lib/yaml.js:78:20)
|
| Comment by Federico Grilli [ 04/Nov/16 ] |
|
Apparently it's yaml-js itself which ignores spaces, line breaks and comments https://github.com/connec/yaml-js/blob/master/lib/scanner.js#L670 Not sure about the consequences (and feasibility) of using a custom Scanner to not skip those. Maybe this rings a bell apchelintcev ? |
| Comment by Aleksandr Pchelintcev [ 05/Nov/16 ] |
|
czimmermann fgrilli tgregovsky The way we inject the snippets of YAML into our files involves the following:
Since neither comments nor the empty lines are part of that tree - they're obviously lost. Can't find it online - but we've had a conversation about such trade-offs back in days and those were not considered as blockers due to how the tool was positioned at that time (provides a jump-start support to get your thing working and then you manually get those into shape). fgrilli regarding the !include causing problems - that might be an actual issue: there should be no problem with this as !include is a simple YAML tag and is supposed to be a part of the element tree. |
| Comment by Christopher Zimmermann [ 06/Nov/16 ] |
|
Could the parser be customized to also note the spaces and comments and create tokens for those - and the dumper dump those out as well? |
| Comment by Christopher Zimmermann [ 06/Nov/16 ] |
|
Agreed not a blocker if its only a problem with spaces and comments. |
| Comment by Aleksandr Pchelintcev [ 07/Nov/16 ] |
|
czimmermann I am afraid making parser preserve whitespaces and comments isn't trivial - it is against the spec and generally parsers don't do that. What we could do theoretically - is take a diff and merge the comments back (I've never tried do similar things especially in JS, but it should be doable, e.g. with smth like this: https://github.com/kpdecker/jsdiff). Some simple logic could be applied - if the hunk is just about removing the lines from the source files - put them back. Regarding the alternative approach which would skip parsing/dumping phases - it might sound reasonable at first, but if we think more about it - we'll anyway will need some sort of an analysis tool that would determine the correct place in which our YAML snippets should be injected. It'll have to consider all the possible syntax quirks and corner cases and in the end we will end up with yet another parser which will probably work worse than the existing one. |
| Comment by Federico Grilli [ 07/Nov/16 ] |
|
I retract my statement about !include custom directive. We do support it. Problem apparently was that I misplaced it. For instance I used something like !include /foo/bar.yaml baz: bar I added a unit test to ensure that https://git.magnolia-cms.com/projects/BUILD/repos/npm-cli/commits/04637f40113d8501fec3773d98fd921cb066ca99 |
| Comment by Christopher Zimmermann [ 08/Nov/16 ] |
|
Thanks for your input apchelintcev - Diff is cool solution. |
| Comment by Aleksandr Pchelintcev [ 08/Nov/16 ] |
|
No probs! Great to see it working! |