Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FXP Rewrite #682

Merged
merged 38 commits into from
Oct 18, 2022
Merged

FXP Rewrite #682

merged 38 commits into from
Oct 18, 2022

Conversation

KaiVolland
Copy link
Contributor

@KaiVolland KaiVolland commented Sep 15, 2022

This replaces xml2js parser with fast-xml-parser.

Parsing of functions and expressions is not yet supported.

@rdewit
Copy link

rdewit commented Sep 19, 2022

@KaiVolland , since you are rewriting the SLD parser, would it make sense to extract the SLD version from the XML if a user has not provided the value explicitly?

The reason that I'm asking is because when we use the SLD parser in our code, we now 'manually' extract it with using the following (also using fast-xml-parser) just to get that version information:

const parserOptions = {
  attributeNamePrefix: '',
  ignoreAttributes: false,
  removeNSPrefix: true
}
const xmlParser = new XMLParser(parserOptions) // fast-xml-parser
const sldObject = xmlParser.parse(responseText) // the SLD coming from an XHR request
const sldVersion = sldObject?.StyledLayerDescriptor?.version

We then feed that version value as option in the constructor of the SLD parser. It seems that automatically extracting it from the SLD would be a logical next step.

If this added functionality does not fit within this PR, I'll happily wait for your work here to be completed and I can then propose that functionality in a separate PR.

@jansule
Copy link
Contributor

jansule commented Sep 19, 2022

since you are rewriting the SLD parser, would it make sense to extract the SLD version from the XML if a user has not provided the value explicitly?

+1 from my side. Personally, I would prefer to have it in a separate PR though.

@KaiVolland KaiVolland marked this pull request as ready for review October 17, 2022 15:22
@KaiVolland KaiVolland requested review from weskamm, jansule and hwbllmnn and removed request for weskamm October 18, 2022 07:08
@marcjansen
Copy link
Contributor

Can you please change the commit message of 385ca38?

@KaiVolland
Copy link
Contributor Author

Can you please change the commit message of 385ca38?

I'm going to squash this PR so the message will be gone.

Copy link
Contributor

@marcjansen marcjansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a thorough review, some remarks. Take what you agree with. Thanks.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
data/slds/1.1/point_simplepoint_oneline.sld Show resolved Hide resolved
data/slds/1.1/point_styledLabel_literalPlaceholder.sld Outdated Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
src/SldStyleParser.ts Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
src/Util/SldUtil.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dnlkoch dnlkoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice @KaiVolland!

src/Util/SldUtil.ts Outdated Show resolved Hide resolved
src/Util/SldUtil.ts Outdated Show resolved Hide resolved
src/Util/SldUtil.ts Outdated Show resolved Hide resolved
src/Util/SldUtil.ts Outdated Show resolved Hide resolved
src/Util/SldUtil.ts Outdated Show resolved Hide resolved
src/Util/SldUtil.ts Outdated Show resolved Hide resolved
src/Util/SldUtil.ts Outdated Show resolved Hide resolved
src/Util/SldUtil.ts Outdated Show resolved Hide resolved
src/Util/SldUtil.ts Outdated Show resolved Hide resolved
src/Util/SldUtil.ts Outdated Show resolved Hide resolved
Co-authored-by: dnlkoch <[email protected]>
Copy link
Contributor

@jansule jansule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way too much to properly review. Anyhow, made a few remarks.

Is there a reason that parsing Expressions is not included in this PR?

src/SldStyleParser.ts Show resolved Hide resolved
src/SldStyleParser.ts Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
src/SldStyleParser.ts Outdated Show resolved Hide resolved
@KaiVolland
Copy link
Contributor Author

Way too much to properly review. Anyhow, made a few remarks.

Is there a reason that parsing Expressions is not included in this PR?

I wanted to keep this PR only for the rewrite.

The Expression parsing will be completly diffrent then the one before as the geostyler-style v6 and v7 changed a lot there. But there is already an issue for this: #697

Copy link
Contributor

@jansule jansule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the remarks! Nice work @KaiVolland

Just a few minor points left

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
* @param tagName The tagname to get.
* @returns An array of object as created by the fast-xml-parser
*/
export function getChildren(elements: any[], tagName: string): any[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unfortunate. But doesn't above line work anyways? It's the same as writing any except that with this notation, we highlight that the output type equals the input type.

src/Util/SldUtil.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants