[BREAKING CHANGE] Major refactor for v4.0

TangoMan75 commented 3 years ago

Hi Arthur,

Sorry for fat pull request but since I pushed backward compatibility breaking changes anyway, I squashed all changes in one commit.

Here's the list of changes I made:

New tools

NetscapeBookmarkDecoder and NetscapeBookmarkEncoder.
BookmarksDataFormatter class to use before export to have bookmarks ordered by tag.
BookmarksDataConverter and LegacyBookmarksDataConverter class to convert old bookmarks to new data format.

Refactor `parseBoolean`

parseBoolean should only accept strings as entry parameter.
parseBoolean should always return boolean.
$item['public'] should either contain true, false or null (when undefined);
Removed integers, data comes from a text file ; It will simply just never contain integer value.

Deprecations

Tagged most methods from NetscapeBookmarkParser for deprecation.

Should be remove in the next major release.

Refactor constructor

Replace parameters from constructor with "$context" array and class constants. This will allow for Open/Close principle enforcement

Undefined values should remain `null`

This is not the decoder job to set default values.

Undefined Title should return `null`

decoder should not set default $item['name'] = 'untitled'.

Undefined Time should return `null`

decoder should not set $item['dateCreated'] = time() as default.

Undefined Note should return `null`

A note equals to an empty string should return null.

Remove default Tags

Default Tags should be removed ; This is not the decoder job to set default values.

Remove default public/private status

Default public/private status should be removed ; This is not the decoder job to set default values. Undefined $item['public'] should return null

Replaced Travis CI with GitHub actions

This allow any contibutor to have the ci pipeline available within their own fork.

Refactor Tests

Full factorisation of Unit tests.

Updated minimum required version

https://www.php.net/supported-versions.php

php 7.1 and 7.2 have reached end of life.

// composer.json
  "require": {
    "php": ">=7.3"
  },

Note that, php 7.3 will be supported until december 2021.

Updated PHPUnit version

Updated PHPUnit to the latest version.

  "require-dev": {
    // ...
    "phpunit/phpunit": "^9.5"
  }

TangoMan75 commented 3 years ago

Hi Arthur,

I understand this is a lot to process all at once, I hope you find the time to merge this PR.

This was actually quite a lot of work, I would be really happy to get your feedback on this.

Regards

ArthurHoaro commented 3 years ago

Hi Mattias, I understand it's frustrating to put a lot of effort into some work and not get any feedback. I'm sorry about that, I just don't really have time for Shaarli these days.

I'm just letting you know that I have started to review this PR, and it looks great! Awesome work! I'll continue this week, because it's pretty massive, and will have a few comments but overall there shouldn't be too much changes. Especially with our test dataset: if previous UT are still passing we can be confident about these changes.

However you did quite some changes about this repo metadata that weren't discussed beforehand, and it would be nice if you could give more context about all of them (code of conduct, etc.).

TangoMan75 commented 3 years ago

Hi Arthur, I understand you've been busy, no problem.

However you did quite some changes about this repo metadata that weren't discussed beforehand, and it would be nice if you could give more context about all of them (code of conduct, etc.).

Yeah, I know... Sorry about that... I got carried away a bit XD

CODE_OF_CONDUCT.md
ISSUE_TEMPLATE/bug_report.md
ISSUE_TEMPLATE/feature_request.md

These are just template text files, we can change them any fashion you want.

I really love new GitHub Action feature, this allow contributors to have the ci pipeline available within their own fork with is a huge benefit for the project... So I removed Travis.

Also I bumped php and phpunit versions...

We can discuss any changes, take your time.

TangoMan75 commented 3 years ago

Thanks for your feedback @ArthurHoaro !

I'll get back to you soon.

TangoMan75 commented 3 years ago

Hi @ArthurHoaro 👋

🙏 Sorry for long pause in the development of my PR. I had very little time to work on it until recently, thank you for your patience.

Unfortunately I just realized new version will not be compatible with PHP versions below 7.3 because I used type hinting profusely. That would be a bummer to revert all that work since Shaarli can still use previous version of the parser, and will have to bump PHP version at some point anyway => PHP supported-versions 👀

TangoMan75 commented 3 years ago

Hi @ArthurHoaro 👋

I had some time to re-work my PR :

I reverted changes to minimum supported php version, and PHPUnit version : Parser will stay compatible with PHP 7.1 Unfortunately PHPUnit coverage feature will not be available because of it though, maybe we can roll it on future release.

Well, I guess this PR is ready to merge, I hope that you find my contribution useful...

Let me know if I missed anything ✌

TangoMan75 commented 3 years ago

Hey @ArthurHoaro ✋

I didn't get news from you for a long time, I hope you're OK

ArthurHoaro commented 2 years ago

Hi @TangoMan75! I'm well, but kinda took a break from Shaarli and open source development. Sorry about that. I'm trying to get back into it now, so I'll run a few tests and merge this, finally!

ArthurHoaro commented 2 years ago

OK well, I didn't re-review every single line of code, but:

everything seems well design and commented
I tested an export on a large-ish file in both previous and new version and besides the key name changes, everything seems to be the same and running smoothly.
All the tests that we had in place seem to have been preserve and ported, which should cover almost all test cases

So let's roll!

TangoMan75 commented 2 years ago

WOW ! That's great !

shaarli / netscape-bookmark-parser