lucaong / minisearch

Tiny and powerful JavaScript full-text search engine for browser and Node
https://lucaong.github.io/minisearch/
MIT License
4.64k stars 133 forks source link

Update MiniSearch.test.js #228

Closed soshal closed 1 year ago

soshal commented 1 year ago

i did some changes to make this code more efficient

lucaong commented 1 year ago

Hi @soshal , thanks for your contribution, it's always appreciated!

However, I am not going to merge this pull request. The reason is that refactoring tests is risky: there is no "test for the test" to ensure that things still work as intended after the change. Therefore, I would not change the existing tests, unless the improvement is really worth it (like fixing a bug, or adding some additional cases).

For example, the original test here was checking some properties of internal fields upon initialization:

expect(ms._documentCount).toEqual(0)
expect(ms._fieldIds).toEqual({ title: 0, text: 1 })
expect(ms._documentIds.size).toEqual(0)
expect(ms._fieldLength.size).toEqual(0)
expect(ms._avgFieldLength.length).toEqual(0)
expect(ms._options).toMatchObject(options)

The modified test skips some of those:

expect(ms._documentCount).toEqual(0);
expect(ms._fieldIds).toEqual({ title: 0, text: 1 });
expect(ms._options).toMatchObject(options);

While some of the removed checks look trivial, removing them is not an improvement, and may open the flank to some bug (for example, forgetting to initialize the _documentIds array).

Additionally, this pull request changes a style convention, adding semicolons on all lines. While semicolons are fine in general, this is not consistent with the existing code style, and makes it hard to scan the diff for real changes.

My suggestion: when you send a pull request to a project, avoid code style changes (like adding/removing semicolons, using different quotes, etc.), as they introduce inconsistencies and make the code hard to review, lowering the chances that the change will be accepted by the project maintainer. Often your editor reformats the code automatically, in this case better to turn off the formatter when you work on other projects' code.

In other words, thanks for sending a pull request, I appreciate the effort. I will not merge it for the reasons above, but this should not discourage you from contributing to MiniSearch and other open source projects in the future :)

Keep up with the good work!

soshal commented 12 months ago

ok @lucaong I learn something new from you , thank you