Factor out commonalities between repeat and retry into a separate file #363
Conversation
This reverts commit b7dab06.
|
@elelel, |
| namespace operators { | ||
| namespace detail { | ||
|
|
||
| namespace retry_repeat_common { |
There was a problem hiding this comment.
I would prefer for the name of the file to match the name of the type or namespace that is introduces. This preference here does not block merge of this PR.
There was a problem hiding this comment.
Could you be more specific on that? Do you have a naming convention approach? Should I rename the namespace into impl-retry-repeat, rename the file into retry_repeat_common, retry-repeat-common, rx-retry-repeat-common?
There was a problem hiding this comment.
rx-retry-repeat-common.hpp follows the pattern for the rest of the files.
There was a problem hiding this comment.
Fixed that. By the way, what does "rx" prefix stands for in the filenames? I thought it was to prefix something that came from Rx world, like operator names in this dir.
| @@ -25,166 +25,46 @@ | |||
| #define RXCPP_OPERATORS_RX_REPEAT_HPP | |||
|
|
|||
| #include "../rx-includes.hpp" | |||
| #include "impl-retry-repeat.hpp" | |||
There was a problem hiding this comment.
rxcpp was structured so that all include dependencies are expressed in rx-includes.hpp and a few of the .hpp it directly includes (sources, operators, etc..). RX-LITE changed the include structure to make the individual operator includes optional and move them later in the dependency graph.
some options are:
- leave this include structure as is.
- new pattern
- move this include to
rx-includes.hpp- will always be included even in RX-LITE when retry and repeat are not
- merge repeat and retry into a single
.hpp- bleeds the implementation details to the user
Without RX-LITE I would choose differently, but I am leaning to keeping this include as is. What do you think?
There was a problem hiding this comment.
I'm new to RxCpp dev and completely devoid of knowledge how the project is structured. Maybe there's some dev doc describing the coding style recommendations for the project? When structuring the files in this PR I based it solely on the facts from my C++ dev experience:
- When creating code of highly composable functionality there has to be a way to reuse common code in hierarchical way, otherwise code duplication occurs. In our case it is polymorphism through static inheritance. This implies deep dependency trees and that there should be some way to refer to single piece of code from two or more other pieces of codes (files). In other words, we have to include the base class from somewhere.
- This is a header-only library, so a flat include structure will sooner or later become toxic for compile time, so it's better not to be a single include dependencies file
There was a problem hiding this comment.
The code is the primary documentation :)
If the same feedback is required on several PRs from different authors I would find a way to document that.
The developer manual was added by a contributor that wanted to record some things that he found helpful, please feel free to expand that with information you think is useful - or put it in code comments - or doxygen comments in the code - or find a better place and structure to store this kind of information.
Thanks!
| using type = observable<invalid_arguments<AN...>, invalid<AN...>>; | ||
| }; | ||
| template<class... AN> | ||
| using invalid_t = typename invalid<AN...>::type; |
There was a problem hiding this comment.
invalid should not be shared. These structs are used to provide the user with better information in compilation errors (the operator and the argument types that failed to resolve to a supported overload of that operator). retry_repeat_common::invalid is less helpful than repeat::invalid
|
|
||
| template <typename State> | ||
| static inline void on_completed(State& state) { | ||
| // JEP: never appeears to be called? |
There was a problem hiding this comment.
nothing to worry about, but I do not think that we need to keep this comment around anymore. :)
Individual template names are used for compil-time error reporting This reverts commit ad430c5.
| @@ -0,0 +1,153 @@ | |||
| #pragma once | |||
|
|
|||
| /*! \file impl-retry-repeat.hpp | |||
|
Thank you! |
This takes out the commonalities between rx-retry.hpp and rx-repeat.hpp into a single file to reduce the LOC number and prevent multiple place same-edits problems (like the one fixed in #360)