generated from SAP/repository-template
-
Notifications
You must be signed in to change notification settings - Fork 125
Doc improvements on 'Custom Builds' #1837
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling Mistakes
- guides/deployment/custom-builds.md:109:91 Unknown word "myfolder"
Generally, for each spelling mistake there are 2 ways to fix it:
- Fix the spelling mistake and commit it.
- The word is incorrectly reported as misspelled → put the word on the project-words.txt list, located in the root project directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling Mistakes
- guides/deployment/custom-builds.md:117:91 Unknown word "myfolder"
Generally, for each spelling mistake there are 2 ways to fix it:
- Fix the spelling mistake and commit it.
- The word is incorrectly reported as misspelled → put the word on the project-words.txt list, located in the root project directory.
chgeo
reviewed
May 21, 2025
Co-authored-by: Christian Georgi <chgeo@users.noreply.github.com>
Co-authored-by: Christian Georgi <chgeo@users.noreply.github.com>
swaldmann
approved these changes
May 21, 2025
Co-authored-by: René Jeglinsky <rene.jeglinsky@sap.com>
Co-authored-by: René Jeglinsky <rene.jeglinsky@sap.com>
joergmann
approved these changes
May 22, 2025
joergmann
approved these changes
May 22, 2025
renejeglinsky
approved these changes
May 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Minor Changes
monospace font
(rather than italics, which is used more for file names)<Config>
tag where appropriateStructural Changes From Manual Testing For CDS9
I added a related snippet for convenient searching of the relevant section, followed by my feedback.
🔴 the following example doesn't really make it clear how the behaviour can be changed or what the options shown in the configuration do exactly. ✅
🔴 How else should we do it then? ✅
The link then leads to an external blog entry about monorepos.
🟡 This feels a bit weird and out of place. There doesn't seem to be an official guide about mono repos, which is probably the reason why this link was used instead. But maybe we can find something else? ✅ (did not find anything more official)
The section then lists various parameters for the
for
parameter.🟡 I am not entirely sure how to feel about this section. While it currently seems to be up to date, it will inherently become outdated at some point and is basically a clone of what
cds build --help
would list under the appropriate section for--for
. Could we instead have the output ofcds build --help
here and make that more explicit instead? We do this similarly for cds-typer.✅ (postponed: /cds-tools/issues/1541)
🟡 The following section shows an example of a configuration done in package.json. This is one of multiple places where we can see this pattern. We have the
<Config>
custom tag to show various ways of including a configuration, aside from package.json nowadays. Could/ should we use this more excessively here? ✅🔴 This needs an example! ✅
The following code blocks shows an outdated version of the postgres build plugin.
🔴 I don't think we should duplicate the code at this point. We link to the
main
branch of the repository containing this plugin just above. I don't see how showing a larger code block that will inherently be outdated without added explanation brings any value to the table. (The section after that does go into detail about some of the methods, but not all. So even if we wanted to use this particular plugin as an illustration of the API, it is just not complete and would benefit more from having the actual API doc included at this point.)Opportunity to include exported type information inside CAPire?! @chgeo
✅ (we discussed it and came to the conclusion to leave it in. Types can follow at a later point in time.)