[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:
dependency
is depended upon by NPMCLI-55 Release CLI 1.0 Closed
relation
is related to NPMCLI-4 Improve yaml modifications Closed
is related to NPMCLI-115 Component availability is messed up u... Closed
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
------
!include seems to cause troubles as well, which is not good indeed

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:

  • parsing YAML text into an element tree,
  • adding the required elements into that tree,
  • dumping it back to YAML text;

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?
Or what about taking a different approach, not converting to element tree and dumping YAML out again - but simply adding the name of the component at the proper line in the file with a more brute force approach?

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!

Generated at Mon Feb 12 04:46:04 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.