Skip to content
This repository was archived by the owner on Oct 11, 2021. It is now read-only.

refactor: raise violated rules as exception#459

Merged
redeboer merged 3 commits intomasterfrom
refactor-Result
Feb 5, 2021
Merged

refactor: raise violated rules as exception#459
redeboer merged 3 commits intomasterfrom
refactor-Result

Conversation

@redeboer
Copy link
Copy Markdown
Member

@redeboer redeboer commented Jan 28, 2021

Violated rules are now raised as an exception. This simplifies the Result class: it only contains a formalism type and a list of StateTransitionGraphs.

@redeboer redeboer added the ⚠️ Interface Changes to the interface label Jan 28, 2021
@redeboer redeboer requested a review from spflueger January 28, 2021 09:02
@redeboer redeboer self-assigned this Jan 28, 2021
@redeboer redeboer marked this pull request as draft January 28, 2021 09:03
@redeboer redeboer changed the title Refactor result refactor: remove Result class Jan 28, 2021
@redeboer redeboer changed the title refactor: remove Result class refactor!: remove Result class Jan 28, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 28, 2021

Codecov Report

Merging #459 (cc65781) into master (22d6f67) will increase coverage by 0.00%.
The diff coverage is 78.37%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #459   +/-   ##
=======================================
  Coverage   90.04%   90.04%           
=======================================
  Files          22       22           
  Lines        3285     3296   +11     
  Branches      827      830    +3     
=======================================
+ Hits         2958     2968   +10     
- Misses        169      170    +1     
  Partials      158      158           
Flag Coverage Δ
unittests 90.04% <78.37%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/expertsystem/reaction/solving.py 91.42% <ø> (ø)
src/expertsystem/reaction/__init__.py 86.53% <78.37%> (-0.39%) ⬇️
src/expertsystem/amplitude/helicity_decay.py 90.34% <0.00%> (+0.77%) ⬆️

@redeboer
Copy link
Copy Markdown
Member Author

redeboer commented Jan 28, 2021

  • Extract an ExecutionInfo class from the QNResult class

@spflueger
I'm trying to find out why b99825d breaks the channel tests

@redeboer
Copy link
Copy Markdown
Member Author

redeboer commented Jan 28, 2021

@spflueger
As a side note, it's also possible to split this PR into:

  1. Decorating of classes with attr.s. I also tried to decorate Topology, but that breaks the tests somehow, so better to tackle that in a separate PR
  2. Refactoring of the Result class

--> #460

@spflueger
Copy link
Copy Markdown
Member

spflueger commented Feb 1, 2021

  • Extract an ExecutionInfo class from the QNResult class

@spflueger
I'm trying to find out why b99825d breaks the channel tests

We already discussed that in private. But for completeness and documentation I comment here as well. So the issue is that invalid QNResults are created (I can't remember the root cause now). Irrelevant of the root cause, the conclusion is, that the design of this Result class has to be changed as a whole. Otherwise the code keeps staying messy somewhere.

@spflueger
Copy link
Copy Markdown
Member

@spflueger
As a side note, it's also possible to split this PR into:

  1. Decorating of classes with attr.s. I also tried to decorate Topology, but that breaks the tests somehow, so better to tackle that in a separate PR
  2. Refactoring of the Result class

--> #460

I would like to see some plan on how to improve the Result class design first. For example maybe attr.s is not needed or things change completely anyways, that it does not help changing the current code into using attr.

@redeboer redeboer force-pushed the refactor-Result branch 3 times, most recently from 887b2c0 to 63aa9d2 Compare February 5, 2021 08:42
@redeboer redeboer changed the title refactor!: remove Result class refactor: raise violated rules as exception Feb 5, 2021
@redeboer redeboer marked this pull request as ready for review February 5, 2021 08:53
@redeboer redeboer merged commit 0f1e5bd into master Feb 5, 2021
@redeboer redeboer deleted the refactor-Result branch February 5, 2021 11:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

⚠️ Interface Changes to the interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants