Skip to content
This repository has been archived by the owner on Dec 15, 2023. It is now read-only.

#68 - Added a ctor to SimpleHex that resolves ambiguity #70

Closed
wants to merge 2 commits into from
Closed

Conversation

abdulowork
Copy link
Contributor

To resolve #68 I added a ctor to SimpleHex that resolves a length ambiguity by assuming the value of the hex string that is passed is a big endian compact (without any leading zeroes) value. I added relevant tests.

…ed relevant tests. Fixed dependencies that were using the old Data ctor that allowed ambiguous hex string.
… compact big endian value and fixes that. Added relevant tests
@0crat 0crat added the scope label Mar 1, 2018
@0crat
Copy link

0crat commented Mar 1, 2018

Job #70 is now in scope, role is REV

@0crat
Copy link

0crat commented Mar 1, 2018

This pull request #70 is assigned to @driver733/z, here is why. The budget is 15 minutes, see §4. Please, read §27 and when you decide to accept the changes, inform @Biboran/z (the architect) right in this ticket. If you decide that this PR should not be accepted ever, also inform the architect.We should be aware that driver733 is on vacation! This ticket may be delayed.

@0crat
Copy link

0crat commented Mar 1, 2018

Manual assignment of issues is discouraged, see §19: -5 points just awarded to @Biboran/z

@0crat
Copy link

0crat commented Mar 1, 2018

It is strongly discouraged to assign jobs to their creators, see §19: -15 points just awarded to @Biboran/z

If `hexValue` is ambiguous a `DescribedError` is thrown
*/
init(hexValue: String) throws {
guard hexValue.count.isEven() else { throw AmbiguousHexStringError(hexValue: hexValue) }
Copy link
Contributor

@driver733 driver733 Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Biboran There must be no code in constructors, including validations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is ok. This object should not be created with invalid data

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rockfridrich Please read this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rockfridrich I agree with @driver733. I'll change the design. I think i'll just dump alpha-design into a separate branch and work on refactoring.

@@ -62,5 +62,89 @@ class HexTests: XCTestCase {
)

}

func testCorrectlyAlignedHexFor1() {
expect{
Copy link
Contributor

@driver733 driver733 Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Biboran Please limit your tests to only one expect. In other words, test only one thing in your tests, such as one method.

You test should have one expect, but it can have nested expects. This is only permissible if you need them to test one thing. You can not use this technique to test different class methods.

`DescribedError` if something goes wrong
*/
convenience init(bigEndianCompactValue: String) throws {
if bigEndianCompactValue.count.isEven() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There must be no code in constructors.

@abdulowork
Copy link
Contributor Author

This was an incorrect design. I'll add refactoring issues.

@abdulowork abdulowork closed this Mar 5, 2018
@0crat 0crat removed the scope label Mar 5, 2018
@0crat
Copy link

0crat commented Mar 5, 2018

Order was finished, quality was "acceptable": +15 points just awarded to @driver733/z

@0crat
Copy link

0crat commented Mar 5, 2018

The job #70 is now out of scope

@0crat
Copy link

0crat commented Mar 5, 2018

Payment to ARC for a closed pull request, as in §28: +10 points just awarded to @Biboran/z

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hex(hex: String) ambiguous alignment
4 participants