TypeFox / yang-lsp

A Language Server for YANG
http://www.yang-central.org
Apache License 2.0
51 stars 13 forks source link

Quotes to wrap XpathExpression should be kept as what it is after serialization #118

Closed huyuwen closed 6 years ago

huyuwen commented 6 years ago

We recently found that all Quotes which used to wrap XpathExpression will be removed after serialization. For example, original, leaf lf2 { type uint8; must '. <= ../lf1'; } After serialization, leaf lf2 { type uint8; must . <= ../lf1; } We double checked with RFC 7950, actually, it is valid according to the 'must' and 'when' definition. But we still propose to wrap XpathExpression with Quotes, or at least keep as what it is. There are a few reasons,

  1. The example in RFC 7950, (ex, must statement and when statement) all wrap with quotes. That means according to the spec, they recommend to have the quotes.
  2. If there are WS in XpathExpression, Quotes can avoid some parse errors. Actually, pyang doesn't accept 'must' or 'when' expression without quotes if there are WS in expression. Pyang will report it as 'error: unterminated statement definition for keyword "must", looking at...'
  3. XpathExpression with quotes has better readability
JanKoehnlein commented 6 years ago

Could you elaborate on the serialization usecase? Do you read in an existing model with an existing XPath or do you create a new UnparsedXpath? Is it in the SerializationIntegrationTest?

huyuwen commented 6 years ago

Hi Jan,

You can refer to the test case and data in #119.

From Yuwen's iPhone

在 2018年5月17日,下午4:33,Jan Koehnlein notifications@github.com<mailto:notifications@github.com> 写道:

Could you elaborate on the serialization usecase? Do you read in an existing model with an existing XPath or do you create a new UnparsedXpath? Is it in the SerializationIntegrationTest?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/theia-ide/yang-lsp/issues/118#issuecomment-389887897, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAxaKXTPr9zu4E5P2mkW_rzTJ3Nats9cks5tzYoogaJpZM4UC_zs.

JanKoehnlein commented 6 years ago

Depends on https://github.com/eclipse/xtext-core/pull/747

JanKoehnlein commented 6 years ago

Fixed

huyuwen commented 6 years ago

This issue has to be re-opened. We found if there has '+' in the path string, serialization will be wrong. Let's still take the xpath-serialize.yang as example. if we define another leaf node in list (lb), leaf lfb2 { type leafref { path "/ytest:c1" + "/ytest:l1"; } } Then the path in lfb2 will be serialized to
{ path "/ytest:c1 "/ytest:l1"; }

JanKoehnlein commented 6 years ago

Fixed

huyuwen commented 6 years ago

Unfortuantely, we found some other strange behaviors.

  1. '/' position is the end of a segament. there will be an unnecessary space ' ' append into that segament. See output example. Input: leaf lfb2 { type leafref { path "/ytest:c1/" + "ytest:l1"; } } Output: leaf lfb2 { type leafref { path "/ytest:c1/ " + "ytest:l1"; } }
  2. segament break at ':', whatever it is at the end or at the beginning of a segament, there will be unnecessary space ' ' append into the segament Input: leaf lfb2 { type leafref { path "/ytest:" + "c1/ytest" + ":l1"; } } Output: leaf lfb2 { type leafref { path "/ytest: " + "c1/ytest " + ":l1"; } }

The space will make xpath totally wrong.

JanKoehnlein commented 6 years ago

Never mind. I've added the missing case and more tests.

JanKoehnlein commented 6 years ago

Hopefully fixed now. Don't hesitate to reopen if you encounter further pecularities.