Skip to content

adt/objects: normalize CRLF in Class.Include.text#176

Closed
jan-jezek wants to merge 1 commit into
jfilak:masterfrom
jan-jezek:fix/class-include-text-crlf-normalization
Closed

adt/objects: normalize CRLF in Class.Include.text#176
jan-jezek wants to merge 1 commit into
jfilak:masterfrom
jan-jezek:fix/class-include-text-crlf-normalization

Conversation

@jan-jezek

Copy link
Copy Markdown
Contributor

Summary

Class.Include.text (used when checking out a class's testclasses, locals_def and locals_imp includes) returned the raw response from ADT, which uses \r\n line endings. The base ADTObject.text (sap/adt/objects.py:723) has long normalized \r\n -> \n before returning; this override was missing the same call.

Symptom (Windows)

sap.cli.checkout opens the target file in text mode (open(filename, 'w', encoding='utf8')). On Windows, text-mode write translates each \n -> \r\n. When the source already contains \r\n, the result on disk is \r\r\n -- every line renders with a blank line under it in VS Code and other editors.

Linux text-mode IO performs no translation, which is why the issue was invisible upstream.

Fix

One-line change: append .replace('\r\n', '\n') to the override, mirroring the pattern already used by ADTObject.text. Same call, same semantics -- safe on Linux (no-op when there are no \r\n sequences).

Files

  • sap/adt/objects.py -- Class.Include.text

The base ADTObject.text already replaces \r\n with \n before returning the source. Class.Include.text (used for class testclasses, locals_def and locals_imp) was missing the same normalization, returning the raw \r\n string from ADT.

On Windows this surfaces when checkout writes the file in text mode (open(..., 'w')): the existing \r\n is treated as \n and translated again, ending up as \r\r\n on disk and rendering as a blank line under every line in editors. Linux text-mode IO does no translation, which is why the bug had not been noticed.

Mirror the long-standing pattern from ADTObject.text (sap/adt/objects.py:723) so both code paths return source with LF-only line endings.
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2e36f461-cf1a-4328-8f22-11e663fc66de

📥 Commits

Reviewing files that changed from the base of the PR and between 555226a and 122dba7.

📒 Files selected for processing (1)
  • sap/adt/objects.py

📝 Walkthrough

Walkthrough

Class.Include.text in sap/adt/objects.py is updated to call .replace('\r\n', '\n') on the fetched response text, normalizing Windows CRLF line endings to Unix LF before returning the include source content.

Changes

CRLF Normalization in Class.Include.text

Layer / File(s) Summary
CRLF-to-LF normalization in Class.Include.text
sap/adt/objects.py
Class.Include.text now applies .replace('\r\n', '\n') to the fetched response text instead of returning it directly, converting Windows CRLF line endings to Unix LF.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: normalizing CRLF to LF in the Class.Include.text property.
Description check ✅ Passed The description comprehensively explains the issue, its platform-specific symptoms, the root cause, and the one-line fix being applied.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

@jfilak

jfilak commented Jun 17, 2026

Copy link
Copy Markdown
Owner

Well done! Can you please ask your agent to:

Add the test case to the file test/unit/test_sap_adt_class.py to the class TestADTClass along with the methods test_adt_class_read_* whic uses the method self.include_read_test. The first argument of the method include_read_test is the mock response which should be

Response(text='* Line 1\r\n* Line2\r\n* Line3\r\n', status_code=200, headers={'Content-Type': 'text/plain; charset=utf-8'})

and the assert should do full source code equality self.assertEqual(sourceCode, '* Line 1\n* Line2\n* Line3\n')

@jfilak

jfilak commented Jun 18, 2026

Copy link
Copy Markdown
Owner

I pushed af62bfd

@jfilak jfilak closed this Jun 18, 2026
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.

2 participants