Skip to content

Commit ccd09c2

Browse files
xtoughlarshp
andauthored
Contribute ABAP Code Review Guidelines (#183)
* Creative commons license it's not code, anyway * Create README.md initial title * Scaffolding (#5) * scaffolding * remove non existing logo * update title * README: additional info for onboarding (#6) * scaffolding #2 (#7) * scaffolding #2 * some first intro content + added VSCode build task * first intro content * Inital diagram Finally got ditaa to work. Plantuml activity diagrams are apparently not supported. Not really convinced yet of the text-based diagrams approach. Co-authored-by: Christoph Pohl <christoph.pohl@sap.com> * change font to sans-serif (#9) * introduction, minor changes (#10) * some more notes (#12) * some more notes * some words regarding impact * mention abapgit restrictions/design * update scenarios * add note regarding steampunk * first attempt at best practices Co-authored-by: Christoph Pohl <christoph.pohl@sap.com> * small formatting issues (#16) * minor changes (#17) * updates (#23) * update deps * updates * upd * closes #15 * upd * small additions and corrections + some official text on (g)CTS Co-authored-by: Christoph Pohl <christoph.pohl@sap.com> * move the static analysis part into the tooling chapter (#25) * move the static analysis part into the tooling chapter * add short abaplint description * show 3 toc levels * updates * upd * small corrections and enhancements on tooling Co-authored-by: Christoph Pohl <christoph.pohl@sap.com> * Update README.md (#31) * Update README.md * Update README.md * fix typos Co-authored-by: Christoph Pohl <christoph.pohl@sap.com> * add title page logo (#33) * add title page logo * add page numbers * Build timestamp * recommend spellchecker * custom dictionary * add this document section (#32) * add this document section * introduction, sections etc. * remove heading, move related work to new chapter * add related work to index * moved to abstract Co-authored-by: Christoph Pohl <christoph.pohl@sap.com> * front page hacks (#34) * update links (#36) * update activity diagram (#35) * update activity diagram * upper case OK * polish scenarios (#37) * should fix #26 , #24 , #14 * needs some more polishing of example for #21 * publish pdf to latest tag (#38) * Update build.yml * Create publish.yml * Update build.yml * Update build.yml * Update publish.yml * dummy, testing (#39) * rename release asset (#40) * mention latest release pdf (#43) * minor changes (#41) * link Gerrit * upd * add todos * upd * minor * upd * update abaplint section * some diagrams * more diagrams * I love diagrams * upd * remove "to" * happy -> happen * Update code_review.adoc (#45) ``todo, wording? remove "qualified" ? Done. `` "gain reputation" -> "developers can actively become more knowledgeable about the codebase" something like that No need to overdo it. Meritocracy is all about reputation. I don't see how anyone could take offence in that. * update url (#46) * related work: add link (#47) * workaround page numbering problem (#52) * adjust headings (#51) * update README.md with automatic build information (#48) * update README.md with automatic build information * Update README.md Co-authored-by: Christoph Pohl <christoph.pohl@sap.com> * abapgit examples (#53) * wip: abapgit examples * abapgit chapter updates * split setup steps in two * scenario updates (#49) * scenario updates * upd * upd * clarify * update deps * minor changes (#54) * name in italics * additional abaplint links * abapGit add note * add link * Move gCTS example to separate section (#56) * fixes from Mike (#58) * README: add "Thanks To" seciton (#57) * check links in build (#59) * check links in build * test * move structure * add link to code review guide in main README.md * prepare latest release links release links should point to SAP/styleguides * use main license * parent license * ignore build and node_modules * Delete LICENSE redundant to ../LICENSE * Update .gitignore moved build and node_modules to top level * README, add AsciiDoc notes * code review improvements * motivation: getting different perspectives * selection: mention central distribution by team leads * add missing space * Automated Review --> Checks * disambiguation of terms VCS, git platforms... Co-authored-by: Lars Hvam <larshp@hotmail.com>
1 parent 7dbe0b4 commit ccd09c2

22 files changed

+2297
-1
lines changed

.github/workflows/build.yml

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
name: build
2+
3+
on: [push, pull_request]
4+
5+
jobs:
6+
build:
7+
runs-on: ubuntu-latest
8+
steps:
9+
- uses: actions/checkout@v2
10+
- name: Use Node.js
11+
uses: actions/setup-node@v2
12+
- run: npm ci
13+
- run: npm test
14+
- name: Upload build artifact
15+
uses: actions/upload-artifact@main
16+
with:
17+
name: index.pdf
18+
path: ./build/index.pdf
19+
retention-days: 7

.github/workflows/publish.yml

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
name: publish
2+
3+
on:
4+
push:
5+
branches:
6+
- main
7+
8+
jobs:
9+
publish:
10+
runs-on: ubuntu-latest
11+
steps:
12+
- name: Checkout
13+
uses: actions/checkout@v2
14+
- name: Use Node.js
15+
uses: actions/setup-node@v2
16+
- run: npm ci
17+
- run: npm test
18+
- name: Run latest-tag
19+
uses: EndBug/latest-tag@latest
20+
- name: Upload binaries to release
21+
uses: svenstaro/upload-release-action@v2
22+
with:
23+
repo_token: ${{ secrets.GITHUB_TOKEN }}
24+
file: ./build/index.pdf
25+
asset_name: abap-code-review-guide.pdf
26+
tag: latest
27+
overwrite: true

.gitignore

+3-1
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
1-
.idea
1+
.idea
2+
build
3+
node_modules

.vscode/extensions.json

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"recommendations": [
3+
"asciidoctor.asciidoctor-vscode",
4+
"streetsidesoftware.code-spell-checker"
5+
]
6+
}

.vscode/settings.json

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"cSpell.words": [
3+
"abap"
4+
]
5+
}

.vscode/tasks.json

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
{
2+
"version": "2.0.0",
3+
"tasks": [
4+
{
5+
"type": "npm",
6+
"script": "test",
7+
"group": {
8+
"kind": "build",
9+
"isDefault": true
10+
},
11+
"problemMatcher": [],
12+
"label": "npm: test",
13+
"detail": "asciidoctor-web-pdf -r asciidoctor-plantuml -r asciidoctor-kroki -d book -D build src/index.adoc"
14+
},
15+
{
16+
"type": "npm",
17+
"script": "install",
18+
"problemMatcher": [],
19+
"label": "npm: install",
20+
"detail": "install dependencies from package"
21+
}
22+
]
23+
}

README.md

+1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ towards code that has a healthy balance between all of these qualities.
2020
## Style Guides
2121

2222
- [**Clean ABAP**](clean-abap/CleanABAP.md)
23+
- [**ABAP Code Reviews**](abap-code-reviews/README.md)
2324

2425
## Continuous Release
2526

abap-code-review/.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
src/*.html

abap-code-review/README.md

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# ABAP Code Review Guide
2+
ABAP Code Review Guide is a collection of ideas, tools, and approaches for implementing ABAP code reviews in various landscapes.
3+
4+
The document is provided as open source under the [CC license](../LICENSE), suggestions and bugfixes are welcome.
5+
6+
Latest release from main can be found out [abap-code-review-guide.pdf](https://github.com/SAP/styleguides/releases/download/latest/abap-code-review-guide.pdf).
7+
8+
## Building Locally
9+
The PDF file can be built locally, if [Node.js](https://nodejs.org/en/) is installed, by running
10+
11+
`npm install && npm test`
12+
13+
## Editing Locally
14+
The document is written in [AsciiDoc](https://asciidoc.org), and can be edited in any text editor.
15+
16+
AsciiDoc lies between [Markdown](https://en.wikipedia.org/wiki/Markdown) and [LaTeX](https://en.wikipedia.org/wiki/LaTeX), allowing more markup than Markdown, but less syntax than LaTeX. AsciiDoc provides possibility for inlining diagrams, easy table of contents, plus PDF generation.
17+
18+
[vscode](https://code.visualstudio.com) with the [AsciiDoc extension](https://marketplace.visualstudio.com/items?itemName=asciidoctor.asciidoctor-vscode) provides preview directly in the editor.
19+
20+
## Automatic Build
21+
Each time a commit is pushed(except from forks), GitHub actions will run, build the PDF and attach it to the actions run as an artifact.
22+
23+
When changes is pushed to the default branch, the [latest release](https://github.com/SAP/styleguides/releases/download/latest/abap-code-review-guide.pdf) will be updated with the latest version of the document.
24+
25+
# Thanks To
26+
* [Christoph Pohl](https://github.com/xtough/), [@sap](https://github.com/sap)
27+
* [Lars Hvam Petersen](https://github.com/larshp), [@heliconialabs](https://github.com/heliconialabs)
42.5 KB
Loading

abap-code-review/images/logo.svg

+163
Loading
+72
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
== abapGit Examples
2+
3+
=== One Way
4+
5+
There are lots of ways to setup code reviews, this section introduces a simple implementation of the <<One Way Synchronization>>, with commits for each task release, the example can be extended according to the needs of each organization.
6+
7+
A sample abapGit setup is provided at link:https://github.com/abapGit/abapgit-review-example[https://github.com/abapGit/abapgit-review-example], it can be installed via abapGit, and works together with GitHub.
8+
9+
==== Setup
10+
11+
GitHub setup required,
12+
13+
. link:https://docs.github.com/en/github/getting-started-with-github/create-a-repo[Create repositories]
14+
. If needed, link:https://docs.github.com/en/github/administering-a-repository/about-protected-branches[setup branch protection]
15+
. link:https://docs.github.com/en/github/administering-a-repository/managing-the-automatic-deletion-of-branches[Enable automatic branch deletion]
16+
17+
Multiple setup steps are required in the development system,
18+
19+
. abapGit development edition link:https://docs.abapgit.org/guide-install.html#install-developer-version[installed]
20+
. Connectivity from the ABAP system to Github, link:https://docs.abapgit.org/guide-ssl-setup.html[SSL setup]
21+
. link:https://docs.abapgit.org/ref-exits.html#create_http_client[Background user authentication]
22+
. Online repositories are link:https://docs.abapgit.org/guide-online-install.html[created] and linked to the development packages
23+
. Enable abapGit link:https://docs.abapgit.org/ref-dot-abapgit.html#write-protected[write protection] for the repos
24+
25+
==== Workflow
26+
27+
For the release of each task, the following steps are performed:
28+
29+
[plantuml,one-way-abapgit,svg,align="center"]
30+
....
31+
@startuml
32+
scale 7/8
33+
skinparam monochrome true
34+
35+
start
36+
:Determine repository;
37+
38+
if (Does branch exist?) then (no)
39+
:Create branch;
40+
else (yes)
41+
endif
42+
43+
:Push changes to branch;
44+
45+
if (Does PR exist?) then (no)
46+
:Create PR;
47+
else (yes)
48+
endif
49+
50+
stop
51+
@enduml
52+
....
53+
54+
For release of request,
55+
56+
. Check PR is released, if not the request cannot be released
57+
58+
// What happens if the PR is released but user wants to deliberately add further tasks (or accidentally does so)? see https://github.com/larshp/abapgit-review-example/issues/12
59+
60+
abapGit works on object level(`R3TR`), while the transport system works on subobject level(`LIMU`), if mirroring transports to git and subobjects exists in multiple transports, the abapGit based example will give an error.
61+
62+
Transports must consistently match the git repositories, an error will be issued if a transport contains objects from multiple git repositories.
63+
64+
The example is provided as a starting point, different organizations have different requirements and work processes, the example can be adjusted to fit any requirements.
65+
66+
=== Two Way
67+
68+
The developer uses the normal UI of abapGit, link:https://docs.abapgit.org/[pushing and pulling] all changes.
69+
70+
If git is the only destination for the code, suggest disabling CTS transports to make it faster to do changes, eg. by developing in local packages.
71+
72+
image::../images/abapgit1_107_0.png[abapGit UI]

0 commit comments

Comments
 (0)