Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

"parse" improve proposal #5

Closed
wants to merge 3 commits into from

Conversation

denizzzka
Copy link

Proposal about #4

How to use:

        auto range = parseXML!dxmlConfig(res); // parseXML imported from dxml
        auto decodedStruct = range.parse.decodeXml!SomeStruct;

@FeepingCreature
Copy link
Collaborator

Looks nice, thank you! Could you throw in a small unittest for it?

@denizzzka
Copy link
Author

done

@FeepingCreature
Copy link
Collaborator

Thanks! I'll take a look on Monday, since I'm on a short holiday now. (yay!) If I don't get back to you by Tuesday, I've lost the notification so please give me a ping by then. :)

@FeepingCreature
Copy link
Collaborator

FeepingCreature commented Sep 29, 2020

Sorry for the late reply!

I meant, could you throw in a small unittest in unittest/text/xml/DecodeTest.d that actually uses this functionality with a dxml range? So it's nailed down and doesn't get removed during refactoring.

Looks good otherwise.

(Also, rebase on master please :) )

@denizzzka
Copy link
Author

(Also, rebase on master please :) )

Can you explain more?

@FeepingCreature
Copy link
Collaborator

FeepingCreature commented Sep 29, 2020

git checkout master; git pull; git checkout <branch>; git rebase master will rewrite your commits to be as if they'd been added after the new master. Right now you're branching off a few commits back, because I've added commits to master after you opened your PR. Don't worry about it though, I can do it for you when merging.

@denizzzka
Copy link
Author

It is a bad omen to use rebase :-)

@denizzzka
Copy link
Author

denizzzka commented Sep 29, 2020

I meant, could you throw in a small unittest in unittest/text/xml/DecodeTest.d that actually uses this functionality with a dxml range? So it's nailed down and doesn't get removed during refactoring.

This can be checked by coverage analyzis

Sorry, I already removed serialized dependency from my srcs, so this PR isn't actual for me.

@denizzzka denizzzka closed this Sep 29, 2020
@FeepingCreature
Copy link
Collaborator

Fair nuf :) Thanks anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants