TypeFox / yang-lsp

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

Concatenated augment/deviation paths #205

Closed andreasjakobik closed 3 years ago

andreasjakobik commented 3 years ago

Hello,

Similar to issue [https://github.com/theia-ide/yang-lsp/issues/113] also paths for augment and deviation statements should be possible to split up arbitrarily in concatenated strings.

This case involves the SchemaNodeIdentifier.

Example:

augment "/if:interfaces/if:interface/ethxrouter6000:ethernet/l"
  + "2servicer6k:service-instance" {

Thanks, Andreas

dhuebner commented 3 years ago

@andreasjakobik Do you have cases where a concatenation part starts with a -? E.g.: augment "/f:c12/f:c22" + "-foo" { }

This is currently not possible because expected ID rule may not start with a -. Is it a kind of limitation we should solve here as well?

dhuebner commented 3 years ago

Note: Same for numbers: augment "/f:c12/f:c2"+"2" + "-foo" { }

I afraid we need to use the same magic as in #113 but cover more broken cases. After all the squashing the resulting char sequence must be of type RULE_ID

andreasjakobik commented 3 years ago

Each identifier follows the ID rules. An identifier may contain both "-" and numbers, and can be split arbitrarily. Does that answer your question?

dhuebner commented 3 years ago

@andreasjakobik Following would work:

augment "/f:c12" + "/f:c22foo-bar" {
    container c33 {
    }
}
augment "/f:c12/" + "f:c22foo-bar" {
    container c34 {
    }
}
augment "/f:c12/f" + ":c22foo-bar" {
    container c35 {
    }
}
augment "/f:c12/f:c2"+ "2foo-bar" {
    container c36a {
    }
}
augment "/f:c12/f:"  + "c22foo-bar" {
    container c36 {
    }
}
augment "/f:c12/f:c22foo-" + "bar" {
    container c38 {
    }
}
augment "/f:c12/f:c22foo" + "-bar" {
    container c37 {
    }
}

Following will not work: augment "/f:c12/f:c2"+"2"+"foo-2bar" { because the currently used lookeahead is to small. But I think "+"2"+" is not what we expect to see in generated files.

The current solution will also change serialization result. The example above will be serialized to:

augment "/f:c12/f:c22foo-bar" {
    container c32 {
    }
}
augment "/f:c12" + "/f:c22foo-bar" {
    container c33 {
    }
}
augment "/f:c12/" + "f:c22foo-bar" {
    container c34 {
    }
}
augment "/f:c12/f:c22foo-bar" {
    container c35 {
    }
}
augment "/f:c12/f:c22foo-bar" {
    container c36a {
    }
}
augment "/f:c12/f:c22foo-bar" {
    container c36 {
    }
}
augment "/f:c12/f:c22foo-bar" {
    container c38 {
    }
}
augment "/f:c12/f:c22foo-bar" {
    container c37 {
    }
}

I hope it's not an issue for you.

andreasjakobik commented 3 years ago

@dhuebner, It looks fine! Thanks, Andreas

dhuebner commented 3 years ago

@andreasjakobik We have a couple of test models but not that much. Would be nice if could test the current state against your real world projects. Than I think we are good to go with a new release :)

andreasjakobik commented 3 years ago

@dhuebner Agree, not sure to what extent models can be shared open source though. Happy to test on a nightly build. We have tons of examples of these broken path statements, also function names can be broken ("current"+"()") or ("substring-" + "before(...)"). Those examples were found in must statements but you get the point.

dhuebner commented 3 years ago

@andreasjakobik Thanks for providing test data, that was very helpful! It was more like a cold shower, but still very helpful.

From 170 errors in the test project you provided I'm now at 0 errors and 1 warning. I also fixed linking for DescendantSchemaNodeIdentifier and in some other cases. It seems like linking issues inside an Xpath expression e.g. in must are not reported is that true?

andreasjakobik commented 3 years ago

@dhuebner Fantastic! We so much look forward to integrate latest yang-lsp in our tools with these great improvements. Glad the test models helped. You are probably right about linking issues, however there is still red markers when open file in Yangster.

dhuebner commented 3 years ago

Fixed and released