Skip to content
This repository was archived by the owner on Jan 21, 2023. It is now read-only.

Better error message on bogus constructor param to elements and other model items #51

Closed

Conversation

yt-ms
Copy link
Collaborator

@yt-ms yt-ms commented Nov 2, 2020

Added check in ModelItem for any unused parameters, and now raises meaningful TypeError listing them.


THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE Apache License v.2.0. YOU MAY OBTAIN A COPY OF THE LICENSE AT https://www.apache.org/licenses/LICENSE-2.0.

THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS

@codecov-io
Copy link

codecov-io commented Nov 2, 2020

Codecov Report

Merging #51 into devel will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            devel      #51      +/-   ##
==========================================
+ Coverage   85.10%   85.15%   +0.05%     
==========================================
  Files          64       64              
  Lines        1745     1751       +6     
  Branches      151      154       +3     
==========================================
+ Hits         1485     1491       +6     
  Misses        228      228              
  Partials       32       32              
Impacted Files Coverage Δ
src/structurizr/model/model_item.py 97.61% <100.00%> (+0.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8c6958...e2ffa0a. Read the comment docs.

Copy link
Owner

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

I agree with the idea but it's slightly trickier than your current solution. The reason I tend to pass keyword arguments into the super's constructor is to enable multiple inheritance.

If you decide against multiple inheritance, an easy solution is to not pass on keyword arguments in classes who inherit from object and silently ignore extra arguments. (As you now basically do with super().__init__() just that you inspect kwargs first.)

However, in multiple inheritance, for example, class C inherits from both classes P1 and P2 which inherit from object, you don't know in which order the MRO will turn out. Thus super() of P1 may be object or P2 and the same is true for P2. Thus you need to pass on keyword arguments in order to properly initialize all of them. That means, extra arguments not consumed by P1 and P2 will be passed on to object which will then complain.

So in summary, we should still use super().__init__(**kwargs) but we can inspect type(self).__mro__ and if object is next in line, we can raise a TypeError as you suggest.

@Midnighter
Copy link
Owner

Closing this one due to #60.

@Midnighter Midnighter closed this Nov 29, 2020
@yt-ms yt-ms deleted the issue-50-better-error-if-bogus-param branch November 29, 2020 21:46
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.

Make error message more intuitive if you get a wrong parameter on an element constructor
3 participants