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

Fix typings #212

Merged
merged 3 commits into from
May 10, 2019
Merged

Fix typings #212

merged 3 commits into from
May 10, 2019

Conversation

sandersn
Copy link
Contributor

@sandersn sandersn commented May 8, 2019

  1. Use correct import syntax for 'stream'
  2. Rename XMLCharacterData -> CharacterData
  3. XMLNode methods return XMLNode instead of XMLElement.

Fixes #211

1. Use correct import syntax for 'stream'
2. Rename XMLCharacterData -> CharacterData
3. XMLNode methods return XMLNode instead of XMLElement.
@coveralls
Copy link

coveralls commented May 8, 2019

Coverage Status

Coverage remained the same at 88.469% when pulling 83ff934 on sandersn:fix-typings into f16faa2 on oozcitak:master.

@oozcitak
Copy link
Owner

oozcitak commented May 9, 2019

I agree on 1 and 2. For 3, a better resolution would be for XMLDocType to not extend XMLNode at all. In that case some methods and properties from XMLNode need to be redeclared in XMLDocType. Could you please modify your PR accordingly:

  • Keep 1 and 2
  • Rollback 3 so XMLNode methods keep returning XMLElement
  • Remove extends XMLNode from XMLDocType
  • Declare the the properties type, parent, children and methods document, doc, end in XMLDocType
  • Also in attList method attributeType and defaultValueType should not be optional, they are both required. The comment header which says these two arguments has default values is wrong.

Thank you for the PR.

Edit: Also for 2 can you please keep the name XMLCharacterData and make XMLText, XMLComment, etc. extend XMLCharacterData instead of CharacterData. Just to keep names consistent across the library.

@sandersn
Copy link
Contributor Author

I like the idea of taking XMLDocType out of the inheritance hierarchy. Let me know if that looks right to you.

@oozcitak
Copy link
Owner

Looks perfect. Thank you. Merging and releasing now.

@oozcitak oozcitak merged commit 690096e into oozcitak:master May 10, 2019
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.

typings are full of errors
3 participants