Skip to content

Cleanup and change constraint names #168

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Cleanup and change constraint names #168

wants to merge 3 commits into from

Conversation

joaopapereira
Copy link
Contributor

In this pull request I am doing the following changes:

The first 2 are part of the issue #52 the last one was something that we observed while using CGreen. The struct Constraint can already be defined in the users application which will cause an error while compiling the test.
To solve this issue the approach was to prefix the structure name with CGreen

@coveralls
Copy link

coveralls commented Sep 27, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 96.357% when pulling 49e08d3 on joaopapereira:cleanup-change-constraint-names into 5c078a6 on cgreen-devs:master.

@joaopapereira
Copy link
Contributor Author

I believe the Shippable error is not related to my commit

@thoni56
Copy link
Contributor

thoni56 commented Oct 4, 2018

True. We have some problems with the Shippable configuration, and I've been meaning to remove it.

@joaopapereira
Copy link
Contributor Author

Did you had some time to review this PR?

matthargett
matthargett reviewed Oct 11, 2018
View reviewed changes
Copy link
Contributor

@matthargett matthargett left a comment

Choose a reason for hiding this comment

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

If Constraint is made a private header, how will users define custom constraints out-of-tree from CGreen? This is pretty common in the 3 companies I deployed cgreen at that did work in *BSD/Linux.

@thoni56
Copy link
Contributor

thoni56 commented Oct 12, 2018 •
edited
Loading

I think @matthargett 's comment need to be considered. We don't want to limit users possibility to extend Cgreens functionality. I did not consider this when I wrote the issue because I had not seen that used anywhere.

Although you would still find the include file and use that, the "internal" part signals that it is not intended for external use, so if that scenario is to be supported, we should keep it where it is.

As a sidenote I think it would be very valuable to have a section in the manual about extending Cgreen with custom constraints. That also signals that this is an intended API. (Think there are also some "API-checking type tests" somewhere which should include this.)

Otherwise I think the name change is a worthwhile change, and also probably necessary if this should be API.

So I suggest to keep the name change of Constraint, but keep constraint.h in the public/API directory.

If constraint_syntax_helper.h was renamed to constraints.h (note the plural) I think that would address the core of the intent of #52, namely that a "normal" user would include constraints.h and that would be all that would be needed.

If you would implement special constraints, you might also need to include constraint.h.

@thoni56 thoni56 mentioned this pull request Oct 12, 2018
Closed
@joaopapereira
Copy link
Contributor Author

I think @thoni56 and @matthargett are right moving constraint.h into internal is hiding the fact that we can create new constraints.
So I will implement the suggestion from @thoni56 and will rename constraint_syntax_helper.h to constraints.h
My only concern would be the name proximity between constraint and constraints but I think we can use the documentation from the story that you created to help better understand each file purpose.

Also noticed that my patch had an inconsistency with names. In the code we use Cgreen.. for Cgreen related types, but my patch was using CGreen so I changed that.

@joaopapereira joaopapereira force-pushed the cleanup-change-constraint-names branch from 7deda01 to f3ea804 Compare October 12, 2018 20:03
@joaopapereira
Copy link
Contributor Author

Any update on the review of this PR?

@joaopapereira joaopapereira force-pushed the cleanup-change-constraint-names branch from f3ea804 to 895d214 Compare December 10, 2018 18:06
@joaopapereira joaopapereira force-pushed the cleanup-change-constraint-names branch from 895d214 to 6491297 Compare December 27, 2018 16:22
@thoni56 thoni56 added this to the 2.0.0 milestone Jan 16, 2019
@joaopapereira
Rename cgreen/constraint_syntax_helpers.h to cgreen/constraints.h
1904746 
Rename the file to `cgreen/constraints.h` so that it is clear this is the
file that should included if we need to use constraints when doing
tests.
If the objective is to create user defined constraints the file that
should be included is `cgreen/constraint.h`.
@joaopapereira
Move function to internal/contraints.h
47c879f 
The function declaration of `get_significant_figures` and
`significant_figures_for_assert_double_are` was moved to
`internal/contraints.h` as the implementation is already in the
corresponding .c file
@joaopapereira
Convert Constraint struct to CgreenConstraint
49e08d3 
The `Constraint` struct name can easily conflict with other application
that have some sort of constraint. To ensure that name collision does
not happen the struct was renamed from `Constraint` ->
`CgreenConstraint`
@joaopapereira joaopapereira force-pushed the cleanup-change-constraint-names branch from 6491297 to 49e08d3 Compare January 29, 2019 22:31
@thoni56 thoni56 force-pushed the master branch 4 times, most recently from 3d2b639 to 35ca914 Compare July 6, 2022 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthargett matthargett matthargett left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

Cleanup include files wrt. constraints
4 participants
@joaopapereira @coveralls @thoni56 @matthargett