🐛 addressing feedback - using const instead of map for location code values

[savitharaghunathan]

fixes #1023

Summary by CodeRabbit

• Refactor
  • Centralized and simplified how Java symbol location types are determined, improving consistency and maintainability across evaluation and symbol listing paths.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Replaced the static locationToCode map with a new public function GetLocationTypeFromString(location string) LocationType that normalizes input and maps strings to LocationType via a switch. Updated two call sites in service_client.go to use the new function instead of direct map lookups.

Changes

Cohort / File(s)	Summary
Location type resolution function external-providers/java-external-provider/pkg/java_external_provider/provider.go	Added GetLocationTypeFromString(location string) LocationType which lowercases input and maps known location strings to LocationType via a switch (falls back to LocationTypeDefault). Removed the prior locationToCode map.
Function call updates external-providers/java-external-provider/pkg/java_external_provider/service_client.go	Replaced map-based lookups with GetLocationTypeFromString(...) in Evaluate() (compute locationCode) and GetAllSymbols() (set "location" argument; explicit int cast applied).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Verify the switch covers all expected location strings and fallback behavior.
• Confirm call sites correctly handle the returned LocationType and the explicit int cast in GetAllSymbols().

Possibly related PRs

• 🐛 creating an enum for the locations to make the code understandable #1021 — Introduces/refactors LocationType/constants and related location handling used by the new function.

Suggested reviewers

• shawn-hurley

Poem

🐰 I hopped through maps and strings today,
Lowercased words led the cleaner way,
A switch now guides each location's fate,
One function steadies the wandering state—
Hooray for tidy code and fewer stray! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title mentions using const instead of map for location code values, which directly matches the PR objective, though the implementation uses a function instead of const values.
Linked Issues check	✅ Passed	The PR addresses issue #1023 by replacing the locationToCode map with a public function GetLocationTypeFromString that handles location type resolution, fulfilling the requirement to refactor map-based approach.
Out of Scope Changes check	✅ Passed	All changes are limited to refactoring location code value resolution in two files, directly aligned with issue #1023 objectives; no unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)

72-107: Add documentation for the public function.

The new GetLocationTypeFromString function is public but lacks godoc documentation. Consider adding a comment describing its purpose, parameters, and behavior.

Apply this diff to add documentation:

+// GetLocationTypeFromString converts a location string to its corresponding LocationType.
+// The input string is normalized to lowercase before matching.
+// Returns LocationTypeDefault for empty strings or unknown location values.
 func GetLocationTypeFromString(location string) LocationType {
 	switch strings.ToLower(location) {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c4e7bdd and 73ca9db.

📒 Files selected for processing (2)

• external-providers/java-external-provider/pkg/java_external_provider/provider.go (1 hunks)
• external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-12-02T02:57:09.802Z

Learnt from: tsanders-rh
Repo: konveyor/analyzer-lsp PR: 1004
File: external-providers/java-external-provider/go.mod:49-50
Timestamp: 2025-12-02T02:57:09.802Z
Learning: In the konveyor/analyzer-lsp monorepo, all external providers (java-external-provider, dotnet-external-provider, generic-external-provider) include a `replace github.com/konveyor/analyzer-lsp => ../../` (or `../..`) directive in their go.mod files. This replace directive is the established pattern for local development within the monorepo and allows providers to reference the local analyzer-lsp module. When providers are built outside the repository, the versioned dependency declared in the require block is used instead.

Applied to files:

• external-providers/java-external-provider/pkg/java_external_provider/service_client.go

📚 Learning: 2025-07-30T12:11:45.673Z

Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.

Applied to files:

• external-providers/java-external-provider/pkg/java_external_provider/provider.go

🧬 Code graph analysis (1)

external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)

external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)

• GetLocationTypeFromString (72-107)

provider/provider.go (1)

• Location (353-356)

🔇 Additional comments (3)

external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)

72-107: LGTM! Clean refactoring to const-based approach.

The switch-based implementation is more maintainable and performant than the previous map approach. The normalization via strings.ToLower() ensures case-insensitive matching, and explicit handling of the empty string case makes the default behavior clear.

external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)

78-78: LGTM! Proper usage of the new helper function.

The call site correctly uses GetLocationTypeFromString to resolve the location type, and the returned LocationType is appropriately used in the switch statement below.

---

123-123: LGTM! Correct usage with explicit int cast.

The explicit int cast is appropriate here since the value is being formatted as a string to pass to the language server. This maintains the same behavior as the previous map-based implementation.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)

72-108: LGTM! Clean refactoring from map to switch-based approach.

The function correctly implements the requested refactoring from issue #1023. The switch statement is more performant and type-safe than the previous map approach, and it covers all 15 location types properly.

Consider these optional enhancements for robustness:

1. Add a godoc comment to document the function's behavior, especially the default return value for unknown inputs:

+// GetLocationTypeFromString converts a location string to its corresponding LocationType constant.
+// The comparison is case-insensitive. Returns LocationTypeDefault for empty or unknown location strings.
 func GetLocationTypeFromString(location string) LocationType {

2. Trim whitespace before processing to handle inputs with leading/trailing spaces:

 func GetLocationTypeFromString(location string) LocationType {
-	switch strings.ToLower(location) {
+	switch strings.ToLower(strings.TrimSpace(location)) {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 73ca9db and 4d08f03.

📒 Files selected for processing (2)

• external-providers/java-external-provider/pkg/java_external_provider/provider.go (1 hunks)
• external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• external-providers/java-external-provider/pkg/java_external_provider/service_client.go

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-07-30T12:11:45.673Z

Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.

Applied to files:

• external-providers/java-external-provider/pkg/java_external_provider/provider.go

📚 Learning: 2025-12-02T02:57:09.802Z

Learnt from: tsanders-rh
Repo: konveyor/analyzer-lsp PR: 1004
File: external-providers/java-external-provider/go.mod:49-50
Timestamp: 2025-12-02T02:57:09.802Z
Learning: In the konveyor/analyzer-lsp monorepo, all external providers (java-external-provider, dotnet-external-provider, generic-external-provider) include a `replace github.com/konveyor/analyzer-lsp => ../../` (or `../..`) directive in their go.mod files. This replace directive is the established pattern for local development within the monorepo and allows providers to reference the local analyzer-lsp module. When providers are built outside the repository, the versioned dependency declared in the require block is used instead.

Applied to files:

• external-providers/java-external-provider/pkg/java_external_provider/provider.go

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

• GitHub Check: build-all-providers (java, java-provider, external-providers/java-external-provider/Dockerfile, J...
• GitHub Check: build-all-providers (c-sharp, c-sharp-provider, Dockerfile, konveyor/c-sharp-analyzer-provider, f...
• GitHub Check: build-all-providers (generic, generic-provider, external-providers/generic-external-provider/Dock...
• GitHub Check: test (windows-latest, windows, amd64)
• GitHub Check: test (ubuntu-latest, linux, arm64)
• GitHub Check: test (macos-latest, darwin, arm64)
• GitHub Check: test (macos-latest, darwin, amd64)
• GitHub Check: test (ubuntu-latest, linux, amd64)
• GitHub Check: benchmark (windows-latest, windows)
• GitHub Check: benchmark (macos-latest, mac)