returnvalue namespace #659

muellerr · 2022-07-26T10:28:38+02:00

muellerr commented

2022-07-26 10:28:38 +02:00

Add new result namespace which contains OK and FAILED returnvalue
Also contains makeCode constexpr function
Mark HasReturnvaluesIF::makeReturnCode deprecated

This prevents from having to implement an interface just to able to use a shorter
version of the general returnvalues.

1. Add new `result` namespace which contains `OK` and `FAILED` returnvalue 2. Also contains `makeCode` constexpr function 3. Mark `HasReturnvaluesIF::makeReturnCode` deprecated This prevents from having to implement an interface just to able to use a shorter version of the general returnvalues.

muellerr added 2 commits 2022-07-26 10:28:39 +02:00

fsfw/fsfw/pipeline/head There was a failure building this commit Details

b827bd8370

update HasReturnvaluesIF

1. Add new retval namespace which contains OK and FAIL returnvalue
2. Also contains makeCode constexpr function
3. Mark HasReturnvaluesIF::makeReturnCode deprecated

This prevents from having to implement an interface just to use a shorter
version of the general returnvalues. A namespace is better suited for this
I think

fsfw/fsfw/pipeline/head Build started... Details

fsfw/fsfw/pipeline/pr-development This commit looks good Details

88ebb67c8d

fix deprecation warnings

muellerr changed title from ~~mueller/expand-retval-if~~ to Expand returnvalue module

2022-07-26 10:29:00 +02:00

muellerr added this to the v6.0.0 milestone 2022-07-26 10:29:10 +02:00

muellerr requested review from gaisser 2022-07-26 19:08:40 +02:00

muellerr requested review from mohr 2022-07-26 19:08:46 +02:00

mohr commented

2022-07-26 20:49:07 +02:00

A class is already a namespace, so I do not see any improvement by creating an additional namespace.

The style of the fsfw is to prefer explicit names which give their intention clearly instead of short ones which might be typed faster (we assume programmers to use an IDE/Editor capable of suggesting names anyway). So, I also see no reason to rename HasReturnvaluesIF to retval and makeReturnCode to makeCode or RETURN_OK/FAILED to OK/FAILED.

The latter two could be argued to not need the return as part of their name as they are scoped within HasReturnvaluesIF, but especially with RETURN_OK/FAILED it is intentional, as they are often used within classes inheriting from HasReturnvaluesIF without namespace specifier and OK as well as FAILED are too generic to be good names.

As it is used at the moment, one could argue ReturnvaluesIF fits better than HasReturnvaluesIF, but I think the difference is so minimal that I would keep it as is.

As a sidenote, I think using retval = HasReturnvaluesIF; is not adding to readability and more or less reducing clarity for the inattentive reader who might miss the using and will not link it to the better known (it is one of the core interfaces of the fsfw after all) HasReturnvaluesIF.

A class is already a namespace, so I do not see any improvement by creating an additional namespace. The style of the fsfw is to prefer explicit names which give their intention clearly instead of short ones which might be typed faster (we assume programmers to use an IDE/Editor capable of suggesting names anyway). So, I also see no reason to rename `HasReturnvaluesIF` to `retval` and `makeReturnCode` to `makeCode` or `RETURN_OK/FAILED` to `OK/FAILED`. The latter two could be argued to not need the *return* as part of their name as they are scoped within `HasReturnvaluesIF`, but especially with `RETURN_OK/FAILED` it is intentional, as they are often used within classes inheriting from `HasReturnvaluesIF` without namespace specifier and `OK` as well as `FAILED` are too generic to be good names. As it is used at the moment, one could argue `ReturnvaluesIF` fits better than `HasReturnvaluesIF`, but I think the difference is so minimal that I would keep it as is. As a sidenote, I think `using retval = HasReturnvaluesIF;` is not adding to readability and more or less reducing clarity for the inattentive reader who might miss the `using` and will not link it to the better known (it is one of the core interfaces of the fsfw after all) `HasReturnvaluesIF`.

muellerr commented

2022-07-27 10:11:08 +02:00

My goal is not to replace the old way but to provide a shorter alternative when writing out a value which is very commonly used across the whole framework, including when writing unittests.

I think HasReturnvalueIF::RETURN_OK is just cumbersome to write, even with IDE support. retval::OK is a lot more convenient and more brief. I prefer to simply write out retval::OK. It is still explicit enough, and there is no environment pollution. Both ways still work.

About the topic with having a shorter way to write this value out, here are how some other libraries do it with general returnvalues :

FreeRTOS: pdPASS and pdFAIL
LwIP: ERR_OK or plain check against 0 and ERR_OK != 0
ESP-IDF: ESP_OK and ESP_FAIL

Rust

res = foo();
res.is_ok();
res.is_err();

Most of the style in the FSFW ressembles Java code with the long names, but I don't have issues with smaller names when it is for values which are commonly used when they are still clear enough. For example,the function System.out.println is explicit enough, but is also kind of annyoing to write, and I prefer the print / println syntax more common to other languages. Kotlin as a Java extension even introduced print again: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.io/print.html

About the usage of the retval namespace: That was something really old, not even directly related to this PR, but was then an issue because of namespace clashes. Apparently I was annoyed by this some time ago and used this namespace to have some more convenience. I also remember providing a shorter way, but I think this was in some breaking way which was why this feature was rejected. The solution above does not break anything now.

I can remove the deprecation warning.

My goal is not to replace the old way but to provide a shorter alternative when writing out a value which is very commonly used across the whole framework, including when writing unittests. I think `HasReturnvalueIF::RETURN_OK` is just cumbersome to write, even with IDE support. `retval::OK` is a lot more convenient and more brief. I prefer to simply write out `retval::OK`. It is still explicit enough, and there is no environment pollution. Both ways still work. About the topic with having a shorter way to write this value out, here are how some other libraries do it with general returnvalues : - FreeRTOS: `pdPASS` and `pdFAIL` - LwIP: `ERR_OK` or plain check against 0 and `ERR_OK` != 0 - ESP-IDF: `ESP_OK` and `ESP_FAIL` - Rust ```rs res = foo(); res.is_ok(); res.is_err(); ``` Most of the style in the FSFW ressembles Java code with the long names, but I don't have issues with smaller names when it is for values which are commonly used when they are still clear enough. For example,the function `System.out.println` is explicit enough, but is also kind of annyoing to write, and I prefer the `print` / `println` syntax more common to other languages. Kotlin as a Java extension even introduced `print` again: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.io/print.html About the usage of the `retval` namespace: That was something really old, not even directly related to this PR, but was then an issue because of namespace clashes. Apparently I was annoyed by this some time ago and used this namespace to have some more convenience. I also remember providing a shorter way, but I think this was in some breaking way which was why this feature was rejected. The solution above does not break anything now. I can remove the deprecation warning.

muellerr commented

2022-07-27 13:14:41 +02:00

Another suggestion: Use result instead of retval as the namespace name

Another suggestion: Use `result` instead of `retval` as the namespace name

muellerr added 1 commit 2022-07-27 21:43:46 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

5355e63711

use result instead of retval

muellerr added a new dependency 2022-07-29 09:52:03 +02:00

#655 Refactor TMTC Stack, improve test framework

muellerr added 1 commit 2022-08-05 14:02:54 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

e6957de166 Merge branch 'development' into mueller/expand-retval-if

muellerr added 1 commit 2022-08-15 14:36:06 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

3b000d924a Merge branch 'development' into mueller/expand-retval-if

muellerr commented

2022-08-15 14:44:14 +02:00

UPDATE:

Agreement reached: Conversion of HasReturnvaluesIF to namespace returnvalue

HasReturnvaluesIF::RETURN_OK -> returnvalue::OK
HasReturnvaluesIF::RETURN_FAILED -> returnvalue::FAILED

For an even shorter version, users can use using returnvalue at the top of the source file.

UPDATE: Agreement reached: Conversion of `HasReturnvaluesIF` to namespace `returnvalue` - `HasReturnvaluesIF::RETURN_OK` -> `returnvalue::OK` - `HasReturnvaluesIF::RETURN_FAILED` -> `returnvalue::FAILED` For an even shorter version, users can use `using returnvalue` at the top of the source file.

muellerr changed title from ~~Expand returnvalue module~~ to WIP: Expand returnvalue module

2022-08-15 14:50:45 +02:00

muellerr added 1 commit 2022-08-15 15:15:53 +02:00