Skip to content

Update: move const to types make code simpler - #204

Merged
vcaesar merged 1 commit into
masterfrom
dev-v1
Mar 12, 2026
Merged

Update: move const to types make code simpler#204
vcaesar merged 1 commit into
masterfrom
dev-v1

Conversation

@vcaesar

@vcaesar vcaesar commented Mar 12, 2026

Copy link
Copy Markdown
Member

Please provide Issues links to:

  • Issues: #

Provide test code:

Description

...

Copilot AI review requested due to automatic review settings March 12, 2026 00:14
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@vcaesar vcaesar added the update label Mar 12, 2026
@vcaesar
vcaesar merged commit f69dd9d into master Mar 12, 2026
8 of 10 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR moves dictionary file type constants and BM25 default constants from the (now removed) consts package into the types package, and updates call sites to reference types.* instead of consts.*.

Changes:

  • Add exported load-dict type constants and BM25 default constants to types/dict_file.go.
  • Update TFIDF/BM25/tag extractor code and tests to use types.LoadDictType* and types.BM25Default*.
  • Remove consts/dict_file.go and adjust header formatting in dict_1.16.go.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
types/dict_file.go Hosts moved constants (load-dict types + BM25 defaults).
hmm/relevance/tfidf.go Switches TFIDF dict loading to use types.LoadDictTypeTFIDF.
hmm/relevance/bm25.go Switches BM25 dict/default constants to types.* (also touches defaulting logic).
hmm/extracker/tag_extracker.go Switches file-type branching to types.LoadDictCorpus / types.LoadDictTypeBM25.
dict_1.16_test.go Updates test to use types.LoadDictTypeTFIDF.
dict_1.16.go Minor comment/header formatting change.
consts/dict_file.go Deleted; prior location of constants.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread types/dict_file.go
// BM25DefaultK1 default k1 value for calculate bm25
BM25DefaultK1 = 1.25

// BM25DefaultK1 default B value for calculate bm25

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment for BM25DefaultB says BM25DefaultK1 default B value..., which is inaccurate/misleading. Please rename the comment to refer to BM25DefaultB (and ideally describe it as the default B parameter for BM25).

Suggested change
// BM25DefaultK1 default B value for calculate bm25
// BM25DefaultB default B parameter value for calculating bm25

Copilot uses AI. Check for mistakes.
Comment thread hmm/relevance/bm25.go
}
if bm25Setting.B == 0 {
bm25Setting.K1 = consts.BM25DefaultB
bm25Setting.K1 = types.BM25DefaultB

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After fixing the defaulting logic for B, it would be good to add a unit test covering NewBM25 defaults (e.g., nil settings and settings with B == 0) to prevent regressions. There are already Go tests in this repo (e.g. dict_1.16_test.go), but BM25 defaulting isn’t currently exercised.

Suggested change
bm25Setting.K1 = types.BM25DefaultB
bm25Setting.B = types.BM25DefaultB

Copilot uses AI. Check for mistakes.
Comment thread hmm/relevance/bm25.go
}
if bm25Setting.B == 0 {
bm25Setting.K1 = consts.BM25DefaultB
bm25Setting.K1 = types.BM25DefaultB

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In NewBM25, when bm25Setting.B == 0 the code assigns BM25DefaultB to bm25Setting.K1 instead of bm25Setting.B. This leaves B at 0 and changes K1 unexpectedly, which will produce incorrect BM25 weights. Set the default on the B field here.

Suggested change
bm25Setting.K1 = types.BM25DefaultB
bm25Setting.B = types.BM25DefaultB

Copilot uses AI. Check for mistakes.
Comment thread types/dict_file.go
Comment on lines +23 to +30
const (
// dict file type to loading

// LoadDictTypeIDF dict of IDF to loading
LoadDictTypeIDF = iota + 1

// LoadDictTypeTFIDF dict of TFIDF to loading
LoadDictTypeTFIDF

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change removes the consts package and moves these exported constants into types. Since the module path is still github.com/go-ego/gse (no v2+ major version), dropping github.com/go-ego/gse/consts is a breaking API change for downstream users. Consider keeping a small consts package that re-exports these constants (possibly with deprecation comments) to preserve backward compatibility.

Copilot uses AI. Check for mistakes.
Comment thread types/dict_file.go
Comment on lines +23 to +27
const (
// dict file type to loading

// LoadDictTypeIDF dict of IDF to loading
LoadDictTypeIDF = iota + 1

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the constants live in types/dict_file.go, several existing field comments in this file still point to consts/dict_file.go for where defaults/types are defined. Please update those references so the documentation matches the new location (and avoids pointing to a deleted package/file).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants