search

vanderlee / phpSyllable

PHP Syllable splitter/counter and Hyphenator for text and HTML. Multi-language, customisable, cached and fast!
http://vanderlee.github.io/phpSyllable/
117 stars 80 forks source link

Added splitWords and various code quality improvements #58

Closed vanderlee closed 1 year ago

vanderlee commented 1 year ago

Fixes: #65

alexander-nitsche commented 1 year ago

Hi Martijn,

i dived into the Syllable class for the first time and made some adjustments regarding this Syllable::splitWord(), splitWords() and splitText(). What remains would be to generate a new PHPDoc for docs/ from the Syllable class. Not sure, how to best achieve that.

As to your splitWord() question: I would keep splitWord() but i would outline in the documentation, that it does handle only simple words, no text punctuation. I added according notes in the code, tests and README.md.

As to the splitWords() concept: The implementation had some bugs that i fixed. In addition there is one decision to make: what is the purpose of splitWords()? For now i separated the text punctuation from the words (see the tests), but this has to be checked against the purpose of this method.

The profiler shows some slight performance gains for the adjusted splitText() implementation, but nothing fundamentally. I did the changes to understand the function and strip it down to the minimum required code lines to improve copy&paste&adapt for splitWords().

I removed the @cover annotations from the test classes again, as they have reduced the lines covered by tests a lot. What is the purpose of adding the covers annotation? It might be error-prone to use it in this package as PHPUnit seems to be perfectly able to link tests to covered code currently.

Last but not least, i accidentally pushed a rebased state to this PR and therefore somehow have overwritten you as committer of the first commit. Sorry for that.

Greetings, Alex

alexander-nitsche commented 1 year ago

Relates to #28 and #57.

alexander-nitsche commented 1 year ago

Closes: #30

alexander-nitsche commented 1 year ago

The code coverage reports with @covers annotation: syllable-test-coverage-with-annotation and without: syllable-test-coverage-without-annotation

alexander-nitsche commented 1 year ago

Hi Martijn,

i have added a documentation generator that will update the API documentation in README.md if 

./build/generate-docs

is executed on the console. You should run it as soon as the Syllable class is changed. I have run it once. Let's try to find a final format for the method signature: in the previous manually maintained API documentation there were some inconsistencies in the format, e.g. adding or skipping the keyword "function", the number of spaces, etc:).

Greetings, Alex

alexander-nitsche commented 1 year ago

Hi Martijn,

i have added a GitHub Action that checks if the API documentation in README.md is up-to-date.

Greetings, Alex

alexander-nitsche commented 1 year ago

Remaining questions from my side would be:

  1. What is the purpose of Syllable::splitWords()?
  2. Do you agree with the API reference format?
alexander-nitsche commented 1 year ago
  1. I would not just rename API variables and methods, like Syllable::setCacheDir() to Syllable::setDirectoryCache and the like, as it will break all uses of the package out there and require at least a minor release.
  2. Maybe Codacy analysis can be disabled / dropped for now, as it currently fails and counteracts StyleCI requirements. Maybe one format guard is sufficient :)?
alexander-nitsche commented 1 year ago

Did some small unifications and beautifications.

alexander-nitsche commented 1 year ago

The latest two commits fix #65 .

alexander-nitsche commented 1 year ago

Hi @vanderlee ,

how about

1) reverting the renaming of API variables and methods of this PR, like Syllable::setDirectoryCache() and the like, to not break all uses of the package out there, and 2) dropping the Codacy analysis,

and getting this PR merged?

Greetings Alex

vanderlee commented 1 year ago

Hi @alexander-nitsche. I'll look into dropping Codacy.

With regards to @covers; I feel coverage is a tool rather than a goal by itself. Removing @covers might report covered lines but does not provide any incentive to tailor testcases to the functionality that those lines represent.

Anything API should probably be reverted for backwards compatibility.

Haven't looked into the API reference format but any formal documentation will be better than the manual stuff we have now.

alexander-nitsche commented 1 year ago

Ok, i will revert the API names and you the Codacy settings and we'll meet here soon again :)

As for @Covers: Let's move this topic to another PR, ok?

vanderlee commented 1 year ago

Once I have some spare time (i.e. not soon), I'll try to improve coverage and reinstate the @covers. For now I'm okay to merge

alexander-nitsche commented 1 year ago

@vanderlee : Should we create another release? If so, minor or patch release? Minor would be fine as we are introducing a new API method splitWords() and patch would be fine, as i do not know yet the use case of splitWords() and maybe it has no use case at all :)

vanderlee commented 1 year ago

According to semver, it should be a minor because we did add a new feature. So 1.6.0