-
Notifications
You must be signed in to change notification settings - Fork 56
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
ocean.core.Verify
#214
ocean.core.Verify
#214
Conversation
src/ocean/core/Verify.d
Outdated
|
||
module ocean.core.Verify; | ||
|
||
import ocean.meta. |
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.
.
?
src/ocean/core/Verify.d
Outdated
|
||
public void verify ( bool ok, lazy istring msg, | ||
istring file = __FILE__, int line = __LINE__ ) | ||
{ |
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.
Empty body?
Ah parts of the 2nd commit need to be squashed into the 1st. |
48387f5
to
1510ade
Compare
Codecov Report
@@ Coverage Diff @@
## v2.x.x #214 +/- ##
==========================================
+ Coverage 68.19% 68.19% +<.01%
==========================================
Files 466 467 +1
Lines 40564 40569 +5
==========================================
+ Hits 27662 27668 +6
+ Misses 12902 12901 -1 |
Oops, should be fixed now. |
src/ocean/core/Verify.d
Outdated
/// | ||
unittest | ||
{ | ||
try |
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.
testThrown
?
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.
Extra import, thus avoided.
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.
Yes, I was just going to say that 👍
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.
Not really a usage example. I'd remove the ///
1510ade
to
6fdb331
Compare
src/ocean/core/Verify.d
Outdated
be used in place of `assert` freely without introducing cyclic imports. | ||
|
||
Copyright: | ||
Copyright (c) 2017 Sociomantic Labs GmbH. |
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.
nitpick, copyright should be verbatim:
Copyright (c) 2017 sociomantic labs GmbH. All rights reserved
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.
(apparently, it's case sensitive)
relnotes/verify.feature.md
Outdated
* `ocean.core.Verify` | ||
|
||
New module with a single function, `verify`, intended to serve as drop-in | ||
replacement for `assert` to comply to Sociomantic assert/enforce policies. |
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.
I would explain the reasons for it a bit more, or at least link to the relevant issue where it is explained.
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.
AFAIK out assert-vs-enforce guide is not available in public place anywhere. Would be nice to do that so it can be linked.
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.
Would https://github.com/sociomantic-tsunami/neptune be a good place for it (or is it purely versioning related)?
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.
Looks like it is only versioning related. Its README says:
Neptune is a set of guidelines and tools to help developers and users to implement a versioning scheme based on SemVer.
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.
Yes, neptune is 100% about out usage of SemVer and related tools. General coding guidelines don't fit there.
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.
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.
Former looks applicable 👍
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.
|
||
*******************************************************************************/ | ||
|
||
public class SanityException : Exception |
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.
I don't want to start a bike shed war, but I wonder why you didn't go for ProgrammingException
or if there was any discussion about the name.
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.
There was no specific discussion, I picked the name because I was already using this name in other places to name things with such purpose. I believe it is a more generic and thus correct term than ProgrammingException
because cases when we would want to use it may also detect architecture issues or serious violations from trusted input source (like DHT).
@@ -30,6 +30,7 @@ import core.thread; | |||
|
|||
import ocean.transition; | |||
import ocean.core.Enforce; |
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.
Is enforce still needed here?
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.
Yep, there are also regular exceptions in scheduler.
6fdb331
to
0b7e0a8
Compare
Updated with a bit more extensive release notes and fixed copyright/ddoc. |
@gavin-norman-sociomantic it was added per your request so please do the honors of final acceptance ;) |
relnotes/verify.feature.md
Outdated
|
||
New module with a single function, `verify`, intended to serve as drop-in | ||
replacement for `assert` to comply to [Sociomantic assert/enforce | ||
policies](https://github.com/sociomantic-tsunami/sociomantic/blob/master/Code/assert_vs_enforce.md) |
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.
Has this url been discussed? As this is at the top level of sociomantic-tsunami, I would have thought some indication of which programming language this relates to would be good.
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.
We'll need to adapt the guidelines to specifically address usage of assert vs enforce vs verify, but we can do that later.
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.
Has this url been discussed?
I expected to get some suggestion how to name it from @dylan-cromwell-sociomantic but he merged it with this URL :P
Adapting guideline is planned after this is merged.
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.
he merged it with this URL :P
404, though...?
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.
Oops, md vs rst. Updating.
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.
Done
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.
@mihails-strasuns-sociomantic @gavin-norman-sociomantic normally I would take more time to discuss the structure but I trust your judgement and simply don't have the bandwidth right now. Please do propose a language-based structure if need be.
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.
👍 We can discuss a better structure. The main thing is that we have it in tsunami now.
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.
Yup, awesome. I think that relying on me (one person) for reviewing structure of things even in that single repo is going to be too much of a bottleneck considering that we would want me to continue to focus on the overall strategy and setup of the program itself. But of course, if you like to discuss or need any feedback at all, I am super happy to comply, just poke me!
0b7e0a8
to
4a650bf
Compare
Starting point for #212