-
Notifications
You must be signed in to change notification settings - Fork 604
Commit
- Loading branch information
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
51ce146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Leonidas-from-XIV with b9b44e0, this really should be a major version bump.
51ce146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
51ce146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Leonidas-from-XIV. I agree with @browndp08.
51ce146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Leonidas-from-XIV technically, sure below 1.0 does not specify behavior, but practically, with 7 million downloads in the last month, should this library still be versioned below 1.0? Maybe you're handling the invalid XML better, but you're also throwing an error when you did not before, so that's most definitely a breaking change. Literally, code broke 😄. This package has been very successful, so congrats on that! That said, you may not have a technical responsibility to make this 1.0.0 or to limit breaking changes from appearing in patch versions, but you may want to consider the practical impacts on your real users.
51ce146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@browndp08 If you check out the huge list of options required to get reasonable behaviour out of the library (because changing it would break code) and the fact that the
parseString
takes callbacks, you can't really blame me for breaking compatibility very often.What I would agree with is the fact that the behaviour of faulty XML being fed into this library is very bad and essentially often random, so instead of one random behaviour you get something hopefully a bit less random.
51ce146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've released 0.4.19 without that commit.
But please make sure your code does not depend on invalid XML being parsed.