Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SKTD] New object type: Knowledge transfer documents #270

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

uenal-akkaya
Copy link
Contributor

No description provided.

@cla-assistant
Copy link

cla-assistant bot commented Dec 5, 2021

CLA assistant check
All committers have signed the CLA.

@schneidermic0
Copy link
Contributor

Thanks @uenal-akkaya. I suggest to convert this PR to a draft. Is this fine for you?

@uenal-akkaya
Copy link
Contributor Author

@schneidermic0 You can convert this to a draft. Thanks.

@schneidermic0 schneidermic0 marked this pull request as draft December 6, 2021 14:56
Fix abaplint issue

Co-authored-by: abaplint[bot] <24845621+abaplint[bot]@users.noreply.github.com>
Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

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

Thanks @uenal-akkaya. I would like to request some changes. See my comments/suggestions.

file-formats/sktd/examples/z_aff_example_sktd.sktd.json Outdated Show resolved Hide resolved
file-formats/sktd/type/zif_aff_sktd_v1.intf.abap Outdated Show resolved Hide resolved
file-formats/sktd/sktd-v1.json Outdated Show resolved Hide resolved
file-formats/sktd/type/zif_aff_sktd_v1.intf.abap Outdated Show resolved Hide resolved
@schneidermic0 schneidermic0 changed the title Feature/sktd [SKTD] New object type: Knowledge transfer documents Dec 8, 2021
@schneidermic0
Copy link
Contributor

@uenal-akkaya Thanks for the update.

The editor configuration shows that there are some whitespaces in empty lines. Are they really necessary. If not, can we remove them?

@uenal-akkaya
Copy link
Contributor Author

@uenal-akkaya Thanks for the update.

The editor configuration shows that there are some whitespaces in empty lines. Are they really necessary. If not, can we remove them?

Not sure which the editor configuration is. Can you name them?
If you mean the z_aff_example_sktd.sktd.en.md, there the whitespaces in the empty lines are from the documentation text where it was typed that way in the editor.

@schneidermic0
Copy link
Contributor

The repository is checked by an editorconfig file. See https://github.com/SAP/abap-file-formats/blob/34ad66189f7e39bb5aa10cb125f8dd90233b4e43/.editorconfig.

The config checks whether there are any unnecessary whitespace in the code, because this might lead to unnecessary diffs if you compare it to new changes.


File | Cardinality | Definition | Schema | Example
:--- | :--- | :--- | :--- | :---
`<name>.sktd.json` | 1 | [`zif_aff_sktd_v1.intf.abap`](./type/zif_aff_sktd_v1.intf.abap) | [`sktd-v1.json`](./sktd-v1.json) | [`z_aff_example_sktd.sktd.json`](./examples/z_aff_example_sktd.sktd.en.json)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong link to the JSON example file.

Suggested change
`<name>.sktd.json` | 1 | [`zif_aff_sktd_v1.intf.abap`](./type/zif_aff_sktd_v1.intf.abap) | [`sktd-v1.json`](./sktd-v1.json) | [`z_aff_example_sktd.sktd.json`](./examples/z_aff_example_sktd.sktd.en.json)
`<name>.sktd.json` | 1 | [`zif_aff_sktd_v1.intf.abap`](./type/zif_aff_sktd_v1.intf.abap) | [`sktd-v1.json`](./sktd-v1.json) | [`z_aff_example_sktd.sktd.json`](./examples/z_aff_example_sktd.sktd.en.json)

Please also add the link to the markdown file.

Comment on lines +19 to +23
# ensure the LF line endings for SKTD markdowns
[*.md]
charset = utf-8
end_of_line = lf
insert_final_newline = true
Copy link
Contributor

@schneidermic0 schneidermic0 Feb 16, 2023

Choose a reason for hiding this comment

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

This configuration must not run for all *.md files. For other *.md files trailing whitespaces shall still be checked

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean, the .md file in the AFF for SKTD is allowed to have trailing whitespaces?

Copy link
Contributor

Choose a reason for hiding this comment

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

Try adding to the original '.editorconfig'

[/file-formats/sktd/**/*.md]
trim_trailing_whitespace = unset

However, having trailing whitespaces might lead to undesired diffs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Yes, you are right. This is also what I mentioned earlier. See #270 (comment)

Currently, @uenal-akkaya tends to keep them anyway. But this is still in discussion ;) If we can avoid this exemption, it would be great. :)

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.

4 participants