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

121 replace assert with check #163

Merged

Conversation

milancurcic
Copy link
Member

@milancurcic milancurcic commented Mar 24, 2020

This PR implements the check subroutine, proposed to replace assert for internal testing. Specifically:

  • Implements check as proposed here
  • Replaces existing calls to assert with calls to check
  • Removes the definition of assert from stdlib_experimental_error

This PR doesn't cover making all check calls to print meaningful messages on failure, and to execute them with warn=.true., to allow multiple test failures in a single run. These will be covered in a future PR.

This internal testing is meant as temporary until we can do it with fpm test as proposed in #162. For now, we consider this as a "good enough" solution that will help us move forward with development while fpm is in the works.

Finally, there is also Brad's proposed implementation here which we shouldn't ignore. If there is some support from the community for it, let's discuss and consider it.

Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

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

That looks good.

Copy link
Member

@ivan-pi ivan-pi left a comment

Choose a reason for hiding this comment

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

I left only two minor comments.

character(*), intent(in), optional :: msg
integer, intent(in), optional :: code
logical, intent(in), optional :: warn
character(*), parameter :: msg_default = 'Test failed.'
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make more sense if the default message is `'Check failed.' just to be consistent with the subroutine name?

Comment on lines 41 to 42
! ! Stops the program with exit code 1 and prints 'Test failed.'
! call check(a == 5)
Copy link
Member

Choose a reason for hiding this comment

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

There is an implicit assumption in the examples that a is not five. It remains to be discussed whether such examples in the docstrings should be self-contained.

@milancurcic
Copy link
Member Author

@ivan-pi You're right. The latest commit fixes that.

Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

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

That looks good to me.

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thank you.

Copy link
Member

@everythingfunctional everythingfunctional left a comment

Choose a reason for hiding this comment

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

I'm ok with it. I think we should still come up with another function that behaves like I suggested. Obviously we'll have to come up with a new name though.

@milancurcic
Copy link
Member Author

Thanks, all. @everythingfunctional, let's do a separate issue and PR for your proposal when you're ready. Merging.

@milancurcic milancurcic merged commit bdd15c8 into fortran-lang:master Mar 25, 2020
@jvdp1
Copy link
Member

jvdp1 commented Mar 27, 2020

@milancurcic Do we want that users use both check and error_stop outside stdlib? OR are these only aimed for stdlib?
If users can use them outside stdlib, then I think it would be good to have some specs (like for other functions) (I can give a start based on the comments in the code). Personally, I could use both outside stdlib ;)

@milancurcic
Copy link
Member Author

@jvdp1 I think so. Sorry, I totally forgot about the spec. That should've been part of the original PR. Please go ahead with a draft spec if you don't mind. Thank you!

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.

5 participants