Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@
# limitations under the License.

import io
import os
from typing import Dict, Any, Optional, List

import pandas as pd

import requests

TIKA_URL = "http://tika:9998/tika"
TIKA_URL = os.environ.get("TIKA_URL", "http://tika:9998/tika")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 No test coverage for configurable URL behaviour
The test-coverage-new-code project rule requires that new or modified business logic includes corresponding unit tests. This change introduces a runtime-configurable code path (env var override of TIKA_URL) with no accompanying tests. A test verifying that os.environ["TIKA_URL"] is picked up correctly (and that the default is used when unset) would guard against regressions where the lookup is accidentally removed or overridden.

Rule Used: New functionality must include corresponding unit ... (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/common/api/internal/extract/pdf/engines/tika.py
Line: 25

Comment:
**No test coverage for configurable URL behaviour**
The `test-coverage-new-code` project rule requires that new or modified business logic includes corresponding unit tests. This change introduces a runtime-configurable code path (env var override of `TIKA_URL`) with no accompanying tests. A test verifying that `os.environ["TIKA_URL"]` is picked up correctly (and that the default is used when unset) would guard against regressions where the lookup is accidentally removed or overridden.

**Rule Used:** New functionality must include corresponding unit ... ([source](.greptile))

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!



def tika_extractor(
Comment on lines +25 to 28

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 TIKA_URL is resolved once at module import time. If TIKA_URL is set in the environment after the module is imported (a common pattern in test suites or dynamic container environments), the change will be silently ignored and the stale value will be used for every subsequent call. Moving the lookup inside the function (or using functools.lru_cache for efficiency) makes the value reflect the environment at the time of each call.

Suggested change
TIKA_URL = os.environ.get("TIKA_URL", "http://tika:9998/tika")
def tika_extractor(
_DEFAULT_TIKA_URL = "http://tika:9998/tika"
def tika_extractor(
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/common/api/internal/extract/pdf/engines/tika.py
Line: 25-28

Comment:
`TIKA_URL` is resolved once at module import time. If `TIKA_URL` is set in the environment *after* the module is imported (a common pattern in test suites or dynamic container environments), the change will be silently ignored and the stale value will be used for every subsequent call. Moving the lookup inside the function (or using `functools.lru_cache` for efficiency) makes the value reflect the environment at the time of each call.

```suggestion
_DEFAULT_TIKA_URL = "http://tika:9998/tika"

def tika_extractor(
```

How can I resolve this? If you propose a fix, please make it concise.

Expand Down