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

Add interface for UUID-related exceptions #340

Merged
merged 5 commits into from
Apr 23, 2021
Merged

Add interface for UUID-related exceptions #340

merged 5 commits into from
Apr 23, 2021

Conversation

shrikeh
Copy link
Contributor

@shrikeh shrikeh commented Sep 8, 2020

Description

As the exception thrown during the creating of an instance of a UUID requires intimate knowledge of the codec/builder relationship, I had a delve, and thought the simplest answer would be to give them an interface. This allows catching a UuidExceptionInterface $e which will keep the necessary knowledge lighter in the enclosing class.

Motivation and context

How has this been tested?

I ran the existing test suite, after seeing you are catching on specific exceptions. Because the use of the interface to catch is optional, it didn't look like I needed to refactor there, as the tests are after the more specific detail of the exception anyway.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have run composer run-script test locally, and there were no failures or errors. (Well there was one but not related, phan complained about Method Ramsey\Uuid\Converter\Number\GenericNumberConverter::fromHex() should return string&numeric but returns string. So it's not my fault ;-)

@codecov
Copy link

codecov bot commented Oct 29, 2020

Codecov Report

Merging #340 (0dfe28a) into master (e5b3699) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #340   +/-   ##
=========================================
  Coverage     97.51%   97.51%           
  Complexity      567      567           
=========================================
  Files            64       64           
  Lines          1532     1532           
=========================================
  Hits           1494     1494           
  Misses           38       38           

@shrikeh
Copy link
Contributor Author

shrikeh commented Apr 21, 2021

@ramsey Oh mighty god of uuids, what do you think?

@ramsey
Copy link
Owner

ramsey commented Apr 23, 2021

Well, I'm not a fan of being acknowledged as a deity. I'm more of a demigod. 😉

I like the feature. I need to decide whether this goes into the next major version or the current 4.x series. Since it introduces an interface and doesn't change one, I think it still fits semver to go into the next minor.

@shrikeh
Copy link
Contributor Author

shrikeh commented Apr 23, 2021

Morning saviour.

While it’s a bit early for more Monty Python quotes, my coffee-withdrawn brain agrees with you that it fits the minor version semver definition, because it’s a backwards compatible feature.

So it really comes down to your preference if you want to include it quickly or wait (and you know of course I’m going to suggest, for purely selfish reasons, getting it into the next minor version).

@ramsey ramsey merged commit 90a87a7 into ramsey:master Apr 23, 2021
@ramsey
Copy link
Owner

ramsey commented Aug 6, 2021

@shrikeh I've released this in v4.2.0. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants