Added splitWords and various code quality improvements #58

vanderlee · 2023-04-01T11:01:05Z

Fixes: #65

Using the explicit @Covers annotation in tests reduces the coverage result because it seems to focus on the manual assignments rather than the automatically detected covered pieces of code. Relying on automatically determined coverage generally appears to be less error-prone.

- Unify wording. - Make clear that splitWord() does not support punctuation. - Add large text test. - Reword variables to become more verbose. - Reduce expression overhead. - splitWords(): Fix algorithm. For example "A syllable is a unit of organization." was split into [ ['A'], ['syl', 'la', 'ble'], ['is', 'ga', 'ni', 'za', 'tion.'], ] - splitWords(): Split text punctuation into specific array. For example "A syllable is a unit of organization." is split into [ ['A'], [' '], ['syl', 'la', 'ble'], [' '], ['is'], [' '], ['a'], [' '], ['unit'], [' '], ['of'], [' '], ['or', 'ga', 'ni', 'za', 'tion'], ['.'] ]

This may be caused by an insufficient pattern in the TeX language file affected or by a bug of Syllable::parseWord().

alexander-nitsche · 2023-04-02T16:32:05Z

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 · 2023-04-02T16:40:46Z

Relates to #28 and #57.

alexander-nitsche · 2023-04-02T17:14:48Z

Closes: #30

alexander-nitsche · 2023-04-02T17:16:01Z

The code coverage reports with @Covers annotation:

and without:

alexander-nitsche · 2023-04-03T16:27:38Z

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 · 2023-04-04T09:57:51Z

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 · 2023-04-04T10:20:09Z

Remaining questions from my side would be:

What is the purpose of Syllable::splitWords()?
Do you agree with the API reference format?

alexander-nitsche · 2023-04-04T13:45:10Z

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.
Maybe Codacy analysis can be disabled / dropped for now, as it currently fails and counteracts StyleCI requirements. Maybe one format guard is sufficient :)?

Previously, only changes to files of the current directory and subdirectories were committed.

- Fix API documentation generation. - Use " = " instead of "=" for assigning the default value.

- Check the API reference in GitHub Action. - Cache Composer dependencies for reuse in different jobs to speed up workflows and save bandwidth. Mind the global warming.

This should be sufficient to find all version-dependent errors. Mind the global warming.

alexander-nitsche · 2023-04-05T09:22:30Z

Did some small unifications and beautifications.

alexander-nitsche · 2023-04-20T10:35:26Z

The latest two commits fix #65 .

- Addition of caching tests. - Support for nested directory structures in the test directory.

This helps to prevent PHP from encoding the version as a high precision floating point number in the cache file, e.g. 1.399999999999 instead of 1.4.

alexander-nitsche · 2023-04-20T11:21:29Z

Hi @vanderlee ,

how about

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
dropping the Codacy analysis,

and getting this PR merged?

Greetings
Alex

vanderlee · 2023-04-20T11:57:04Z

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 · 2023-04-20T12:02:54Z

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?

Revert it to not break all uses of the package out there. Updated README.md via ``` ./build/generate-docs ```

alexander-nitsche

Ready to be merged, from my side.

vanderlee · 2023-04-20T12:50:51Z

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 · 2023-04-20T13:40:58Z

@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 · 2023-04-20T14:53:26Z

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

Martijn van der Lee and others added 2 commits April 2, 2023 17:24

Added splitWords and various code quality improvements

bd2a8e8

alexander-nitsche force-pushed the master_splitWords branch from f701ad7 to 1e16ca6 Compare April 2, 2023 15:25

alexander-nitsche added 2 commits April 2, 2023 18:03

Add test for collection of words with reduced hyphenation

7b1a640

This may be caused by an insufficient pattern in the TeX language file affected or by a bug of Syllable::parseWord().

alexander-nitsche force-pushed the master_splitWords branch from 1e16ca6 to 7b1a640 Compare April 2, 2023 16:04

Satisfy StyleCI

6362f5b

Add words with reduced hyphenation to collection from PR #26

8e9e5f2

alexander-nitsche force-pushed the master_splitWords branch 2 times, most recently from 03f705a to f8ffcf3 Compare April 4, 2023 09:53

alexander-nitsche force-pushed the master_splitWords branch from f8ffcf3 to 5cf8020 Compare April 4, 2023 10:01

alexander-nitsche self-requested a review April 4, 2023 10:04

alexander-nitsche force-pushed the master_splitWords branch from 5cf8020 to aeb7623 Compare April 4, 2023 10:13

alexander-nitsche force-pushed the master_splitWords branch from aeb7623 to 5aa221e Compare April 4, 2023 10:29

alexander-nitsche added 7 commits April 5, 2023 11:12

Support for generation of API documentation in README.md

492f915

Commit changed files of entire working tree in build context

e0ea164

Previously, only changes to files of the current directory and subdirectories were committed.

Satisfy StyleCI

4cd1b5a

Fix API documentation of an array as parameter default value

ef7b5d0

- Fix API documentation generation. - Use " = " instead of "=" for assigning the default value.

Update API reference

5267ac7

Check via GitHub Action if the API documentation is up-to-date

94ccc52

- Check the API reference in GitHub Action. - Cache Composer dependencies for reuse in different jobs to speed up workflows and save bandwidth. Mind the global warming.

Reduce the PHP test matrix to the latest versions of PHP 5, 7 and 8

7835658

This should be sufficient to find all version-dependent errors. Mind the global warming.

alexander-nitsche force-pushed the master_splitWords branch from 5aa221e to 7835658 Compare April 5, 2023 09:20

alexander-nitsche added 2 commits April 20, 2023 12:42

Cover caching with tests

2e6ddc6

- Addition of caching tests. - Support for nested directory structures in the test directory.

Use cache version as string instead of number

f3841ad

This helps to prevent PHP from encoding the version as a high precision floating point number in the cache file, e.g. 1.399999999999 instead of 1.4.

alexander-nitsche force-pushed the master_splitWords branch from f3b9371 to f3841ad Compare April 20, 2023 10:43

TASK: Revert renaming of API method names

44b4a7b

Revert it to not break all uses of the package out there. Updated README.md via ``` ./build/generate-docs ```

alexander-nitsche approved these changes Apr 20, 2023

View reviewed changes

vanderlee merged commit 275df07 into master Apr 20, 2023

vanderlee deleted the master_splitWords branch April 20, 2023 12:51

vanderlee mentioned this pull request Apr 20, 2023

Add function splitWords() and a test for the function #30

Closed

Added splitWords and various code quality improvements #58

Added splitWords and various code quality improvements #58

Uh oh!

Conversation

vanderlee commented Apr 1, 2023 • edited by alexander-nitsche Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-nitsche commented Apr 2, 2023

Uh oh!

alexander-nitsche commented Apr 2, 2023

Uh oh!

alexander-nitsche commented Apr 2, 2023

Uh oh!

alexander-nitsche commented Apr 2, 2023

Uh oh!

alexander-nitsche commented Apr 3, 2023

Uh oh!

alexander-nitsche commented Apr 4, 2023

Uh oh!

alexander-nitsche commented Apr 4, 2023

Uh oh!

alexander-nitsche commented Apr 4, 2023

Uh oh!

alexander-nitsche commented Apr 5, 2023

Uh oh!

alexander-nitsche commented Apr 20, 2023

Uh oh!

alexander-nitsche commented Apr 20, 2023

Uh oh!

vanderlee commented Apr 20, 2023

Uh oh!

alexander-nitsche commented Apr 20, 2023

Uh oh!

alexander-nitsche left a comment

Choose a reason for hiding this comment

Uh oh!

vanderlee commented Apr 20, 2023

Uh oh!

alexander-nitsche commented Apr 20, 2023

Uh oh!

vanderlee commented Apr 20, 2023

Uh oh!

Uh oh!

vanderlee commented Apr 1, 2023 •

edited by alexander-nitsche

Loading