Skip to content

Conversation

sanmai
Copy link
Contributor

@sanmai sanmai commented Sep 14, 2022

No description provided.

@michelf michelf merged commit 4151738 into michelf:lib Sep 14, 2022
@michelf
Copy link
Owner

michelf commented Sep 14, 2022

Thank you.

@sanmai sanmai deleted the patch-1 branch September 15, 2022 03:10
@sanmai
Copy link
Contributor Author

sanmai commented Sep 20, 2022

@michelf Thanks for considering this PR! Can we have a new release ready with this and other changes?

I'll be happy to give it a go and report back with any new issues.

I could also give you a hand with anything else you need to get it moving.

@michelf
Copy link
Owner

michelf commented Sep 20, 2022

I guess what remains would be to pick a version number, and write the change log in Readme.md.

I should be able to do that by tomorrow.

@sanmai
Copy link
Contributor Author

sanmai commented Sep 21, 2022

I reviewed the changes since the last release, and it looks like the plan you had with 2.0 is a fair bet: there are properties that could no longer accept mixed types, which looks like a BC break to me.

@michelf
Copy link
Owner

michelf commented Sep 21, 2022

Oh, didn't think about that actually. I was planning on deciding between 1.9.2 and 1.10.0 after looking at the changes... but changing the major version just for type annotations makes me rethink whether those type annotations are really worth the trouble. 🤔

@sanmai
Copy link
Contributor Author

sanmai commented Sep 22, 2022

It works like this. If a caller has declare(strict_types = 1); where they use this library, if they assign an inappropriate value to a strictly typed property:

$obj = new \Michelf\MarkdownExtra();
$obj->no_markup = 1;

They will get a TypeError: Cannot assign int to property Michelf\Markdown::$no_markup of type bool so although it's a problem on the caller's side (they could remove strict_types declaration), they couldn't simply composer update and expect everything to work as before. Therefore, unfortunately this looks like a breaking change warranting a new major version.

@sanmai
Copy link
Contributor Author

sanmai commented Sep 22, 2022

To be clear, I don't have anything against a new major version. It is something that has to be done, that's it.

@sanmai sanmai mentioned this pull request Sep 22, 2022
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.

2 participants