Skip to content

doc: adaption#88

Open
Schmarvinius wants to merge 4 commits into
mainfrom
doc/adaption-to-new-strcture
Open

doc: adaption#88
Schmarvinius wants to merge 4 commits into
mainfrom
doc/adaption-to-new-strcture

Conversation

@Schmarvinius

@Schmarvinius Schmarvinius commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Documentation Improvements and Clarifications

Documentation

📝 Improved and corrected documentation across multiple README files, clarifying prerequisites, AI Core binding setup, the AICore CDS service internals, the programmatic API, and integration test profiles.

Changes

  • README.md: Added Maven 3.6.3+ as a prerequisite, removed the outdated Node.js global install requirement, clarified the hermetic build note, updated the integration-test command from mvn test -pl integration-tests/spring -am to mvn verify, and added a reference to the integration-tests README.

  • cds-feature-ai-core/README.md: Clarified that the AICore CDS service is internal (@protocol: 'none') and not exposed via OData. Updated the AI Core binding section to replace bullet-list binding methods with a focused CAP CLI example. Expanded the entity table to reflect UPDATE operations and the stop bound action. Replaced the old AICoreService programmatic usage section with a fully working step-by-step RemoteService-based code snippet. Added a new Public API table documenting ResourceGroupContext, DeploymentIdContext, InferenceClientContext, and ModelDeploymentSpec. Removed the outdated ## Programmatic Usage section that referenced the removed AICoreService facade.

  • cds-feature-recommendations/README.md: Replaced the single model-deduplication note with a two-option guide (Option A: Java-internal only; Option B: OData exposure). Clarified the @cds.odata.valuelist annotation derivation. Fixed a configuration YAML key from requires: to ai:. Added supported CDS HANA types to the field type table. Corrected the mock client name from MockAIClient to MockRecommendationClient. Minor whitespace/trailing-space fixes.

  • integration-tests/README.md: Updated the "skip integration tests" command from the non-existent skip-integration-tests profile to the correct -P-with-integration-tests deactivation syntax. Updated the profiles table accordingly. Lowered cds-feature-recommendations coverage thresholds to 0% with an explanatory note about tightening them once APIs stabilise.

  • 🔄 Regenerate and Update Summary
PR Bot Information

Version: 1.26.0

  • Event Trigger: pull_request.opened
  • Output Template: Default Template
  • Correlation ID: b52a3f81-a40d-4c47-84c7-8ebaa3f13837
  • Summary Prompt: Default Prompt
  • LLM: anthropic--claude-4.6-sonnet
  • File Content Strategy: Full file content

@Schmarvinius Schmarvinius requested a review from a team as a code owner June 19, 2026 06:59

@hyperspace-insights hyperspace-insights Bot 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.

The PR contains one concrete correctness issue worth addressing: the cds-feature-ai-core/README.md programmatic API example imports RptModelSpec from cds-feature-recommendations, a separate module not bundled with cds-feature-ai-core alone — readers following the example without that dependency on the classpath will get a compile error. The other changes are clear documentation improvements with no other substantive issues.

PR Bot Information

Version: 1.26.0

  • Event Trigger: pull_request.opened
  • LLM: anthropic--claude-4.6-sonnet
  • File Content Strategy: Full file content
  • Correlation ID: b52a3f81-a40d-4c47-84c7-8ebaa3f13837

import com.sap.cds.feature.aicore.api.ResourceGroupContext;
import com.sap.cds.feature.aicore.generated.cds4j.aicore.AICore_;
import com.sap.cds.services.cds.RemoteService;
import com.sap.cds.feature.recommendation.api.RptModelSpec;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: Wrong import package for RptModelSpec — the actual source file lives under com.sap.cds.feature.recommendation.api (as confirmed by the repo tree), but the import shown here uses com.sap.cds.feature.recommendation.api — actually the diff shows com.sap.cds.feature.recommendation.api.RptModelSpec yet the package directory is com/sap/cds/feature/recommendation/api/RptModelSpec.java. The import on line 79 uses com.sap.cds.feature.recommendation.api.RptModelSpec which is correct, but the introductory sentence on line 71 says the plugin exposes "three event contexts" — ResourceGroupContext, DeploymentIdContext, and InferenceClientContext — yet RptModelSpec (from a different module, cds-feature-recommendations) is imported without any note that it requires adding cds-feature-recommendations as a dependency. A reader using only cds-feature-ai-core will get a compile error. Add a note that RptModelSpec comes from cds-feature-recommendations, or replace it with the lower-level ModelDeploymentSpec which belongs to cds-feature-ai-core.

Suggested change
import com.sap.cds.feature.recommendation.api.RptModelSpec;
import com.sap.cds.feature.aicore.api.ModelDeploymentSpec; // from cds-feature-ai-core
// import com.sap.cds.feature.recommendation.api.RptModelSpec; // only if cds-feature-recommendations is on the classpath

Double-check suggestion before committing. Edit this comment for amendments.


Please provide feedback on the review comment by checking the appropriate box:

  • 🌟 Awesome comment, a human might have missed that.
  • ✅ Helpful comment
  • 🤷 Neutral
  • ❌ This comment is not helpful

| Profile | Scope | Effect |
|---------|-------|--------|
| _(default)_ | Root | Builds all modules; runs spring integration tests |
| `with-integration-tests` | Root | **Active by default**; includes `integration-tests/` and `coverage-report/`. Deactivate with `-P-with-integration-tests`. |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The "Deactivate with" hint in the table cell is misleading — -P-with-integration-tests deactivates the profile, but the text reads "Deactivate with -P-with-integration-tests", which correctly describes deactivation syntax yet contradicts the column header "Effect". More critically, the profile name with-integration-tests uses a leading - for deactivation, but writers might accidentally copy the string literally into a Maven command. This is fine as written; however the removed skip-integration-tests row is referenced nowhere else — confirm skip-integration-tests is truly gone from the root pom.xml so readers don't try to use it.


Please provide feedback on the review comment by checking the appropriate box:

  • 🌟 Awesome comment, a human might have missed that.
  • ✅ Helpful comment
  • 🤷 Neutral
  • ❌ This comment is not helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant