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

error: splitted error_message document, added error message format #2684

Merged
merged 5 commits into from May 13, 2019

Conversation

Projects
None yet
3 participants
@Piankero
Copy link
Contributor

commented May 9, 2019

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330 please review my pull request

Merging

Please add the "ready to merge" label when the build server and you say
that everything is ready to be merged.

@Piankero Piankero requested a review from markus2330 May 9, 2019

@Piankero

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

jenkins build libelektra please

@Piankero

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@markus2330 does the build server cache README.md documents? The build failure says it cannot find a link when I clearly removed it.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Maybe there is a link to the now non-existing document?

@Piankero

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

error: Failed to find link to error_message.md in the file /home/jenkins/workspace/libelektra_PR-2684-I4G4C7O3FAKCIOP7INAZYROCYG3USUUVQMDO2WSLRQGR7T3U2XJA/doc/decisions/README.md

testscr_check_doc.sh RESULTS: 44 test(s) done 1 error(s).

There is clearly no error_message.md in the decision/README.md file anymore. And the other file exists

@markus2330
Copy link
Contributor

left a comment

Thank you for the PR! Unfortunately, the docu is very messy: title/file names are not consistent, links missing, ...

@@ -0,0 +1,126 @@
# Error message & handling concept

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor

Name of the file does not match with this heading.


## Constraints

- Error numbers must stay because they are more reliable to check against than strings

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor

So we keep error numbers? Why is #2686 then changing int -> char*?


- Error numbers must stay because they are more reliable to check against than strings
- Supporting multiple programming languages
- Plugin System

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor
Suggested change
- Plugin System
- Supporting Elektra's Plugin System

## Decision

All "fatal" errors will be converted to "errors" as the distinguishment is not relevant.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor
Suggested change
All "fatal" errors will be converted to "errors" as the distinguishment is not relevant.
All "fatal" errors will be converted to "errors" as the distinction is not relevant.

All "fatal" errors will be converted to "errors" as the distinguishment is not relevant.

Unused marked errors will be removed from the specification.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor
Suggested change
Unused marked errors will be removed from the specification.
Unused errors will be removed from the specification.

Current errors will be migrated.

## Related Decisions

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor

Links to the other decisions is missing. How do they interrelate?

@@ -0,0 +1,72 @@
# Error message & handling concept

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor

Again the title here does not match the file name (and also not the content).

## Considered Alternatives

Possible variations on what message should be displayed,
eg. to keep the mountpoint information or on how wordings should be (with or without

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor
Suggested change
eg. to keep the mountpoint information or on how wordings should be (with or without
e.g., to keep the mountpoint information or on how wordings should be (with or without

Possible variations on what message should be displayed,
eg. to keep the mountpoint information or on how wordings should be (with or without
"Sorry, ...", coloring of certain parts of a message, etc.)

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor

Make a list of considered alternatives.

This comment has been minimized.

Copy link
@Piankero

Piankero May 10, 2019

Author Contributor

The alternatives are pretty much endless on how you can rearrange an error message. I added some examples but I think it's is not feasible to list all possible variations of what is possible for every field.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor

The list of alternatives is always endless. It is about listing the useful but nevertheless discarded alternatives.

The new error message is much more succinct which gives end users more relevant information.
Furthermore the solution approach still holds all necessary information if requested by users.

`Ingroup`, `Description` and `Module` will be removed from the error message as they provide no useful

This comment has been minimized.

Copy link
@markus2330

markus2330 May 10, 2019

Contributor

This is Implication not Rationale? Description needs to be ported into Reason.

@sanssecours

This comment has been minimized.

Copy link
Member

commented May 10, 2019

There is clearly no error_message.md in the decision/README.md file anymore. And the other file exists

The problem here is that the file error_message.md still exists, while README.md contains no link to this file. You can fix this error by removing error_message.md.

@Piankero Piankero requested a review from markus2330 May 10, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

I have already added my review. Please carefully bring the whole docu up-to-date.

@Piankero

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

I have already added my review. Please carefully bring the whole docu up-to-date.

Beside the alternatives I changed everything you wrote. I now also wrote more alternatives

@markus2330 markus2330 merged commit 24f3278 into ElektraInitiative:master May 13, 2019

11 of 12 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
bsd FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
bsd FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linux Task Summary
Details
mac Task Summary
Details
mac ASAN_OPTIONS:detect_leaks=1 BINDINGS:cpp ENABLE_ASAN:ON TOOLS:kdb Task Summary
Details
mac BUILD_FULL:ON BUILD_SHARED:OFF Task Summary
Details
mac KDB_DB_FILE:default.mmap KDB_DB_INIT:elektra.mmap KDB_DEFAULT_STORAGE:mmapstorage Task Summary
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Thank you.

@Piankero Piankero deleted the Piankero:error-message-format branch May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.