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

Refactor of macro names related to strings and substrings check, to avoid confusion #7

Closed
stefano-p opened this issue Jul 6, 2021 · 11 comments

Comments

@stefano-p
Copy link
Contributor

Hello @jasmcaus ,
I decided to contribute to this unit test framework because I am really going to use it in practice for all my projects. I found it here and I chose it among the others due to its simplicity and effectiveness.
So, I like it very much, but in my opinion there is one aspect that absolutely it is worth to improve. If we consider CHECK_EQ and CHECK_NE as the opposite statement, as well as CHECK_BUF_EQ and CHECK_BUF_NE, I would expect that for strings comparison CHECK_STREQ would be coupled with CHECK_STRNE. Unfortunately, CHECK_STRNE is already used to compare two substrings that are supposed to be equal. In other words, the pattern "_EQ/_NE" is broken.
As such, in order to avoid confusions and keep the whole structure intuitive, I would propose to rename the macros related to strings and substrings as indicated below:

Whole-string Checks:

CHECK_STREQ --> CHECK_STR_EQ
CHECK_STRNEQ --> CHECK_STR_NE
REQUIRE_STREQ --> REQUIRE_STR_EQ
REQUIRE_STRNEQ --> REQUIRE_STR_NE

Substring Checks:

CHECK_STRNE --> CHECK_STRN_EQ
CHECK_STRNNE --> CHECK_STRN_NE
REQUIRE_STRNE --> REQUIRE_STRN_EQ
REQUIRE_STRNNE --> REQUIRE_STRN_NE

As an alternative solution for the substring checks (but this has still to be evaluated) we could allow an optional third parameter in the string check macro. Something like this: CHECK_STR_EQ(actual, expected, n) is like calling CHECK_STRN_EQ(actual, expected, n)

Just to make this transition smooth I would also keep the old names as "deprecated" defines at the bottom of tau.h, so that they can be removed safely at some point in the future.
What do you think?
If you agree, I would be more than happy to take care of this in my repository and then submit a pull request.

Stefano

@stefano-p stefano-p changed the title Refactor of macro names related to strings and substrings, to avoid confusion Refactor of macro names related to strings and substrings check, to avoid confusion Jul 6, 2021
@jasmcaus
Copy link
Owner

jasmcaus commented Jul 7, 2021

Yup, this has been on my mind for quite some time. I generally want to stay away from >2 _s in the macro name (this couldn't be helped for {CHECK|REQUIRE}_BUFF_*).

So, here's my workaround:

Whole-string Checks:

CHECK_STREQ --> CHECK_STREQ
CHECK_STRNEQ --> CHECK_STRNE
REQUIRE_STREQ --> REQUIRE_STREQ
REQUIRE_STRNEQ --> REQUIRE_STRNE

Substring Checks:

CHECK_STRNE --> CHECK_SUBSTREQ
CHECK_STRNNE --> CHECK_SUBSTRNE
REQUIRE_STRNE --> REQUIRE_SUBSTREQ
REQUIRE_STRNNE --> REQUIRE_SUBSTRNE

Adding the SUB hopefully makes it more interpretable

@stefano-p
Copy link
Contributor Author

With your suggested workaround we have for sure solved the ambiguity around the strings and substrings check, which was the main goal of this ticket.
However, following your proposed format, in order to have only one '_' in a macro, then also {CHECK|REQURE}BUF* should become {CHECK|REQURE}_BUF*. I think this will bring a reduced readability, especially if we wanted to add further check macros in the future. In fact, I see 3 important key words that should be clearly visible to the developer:

  • the check/require statement
  • the object (STR, SUBSTR, BUF, ...)
  • the condition (EQ, NE, LT, LE, GT, GE, TRUE, FALSE, NULL, NOT_NULL)
    Please note that in priciple even the conditions LT, LE, GT, GE could be applied to strings/substrings, in the future. Merging the object with the condition make the whole macro less intuitive for the developer and a bit harder to remember.

In my view, such macros could contain a maximum number of 2 '_' characters according to the following logic:

  • 0 '_' chars, when neither the object nor the condition is specified in the name (like CHECK() or REQUIRE());
  • 1 '_' char, where only the object is not specified in the name (i.e. the {CHECK|REQURE}_EQ );
  • 2 and no more than 2 '_' chars where we have both the object and the condition, that for clarity are separated;

That been said, I hope you'll consider also this perspective, aiming to find a compact and intuitive format, which possibly could also be graceful and easy to remember.

@jasmcaus
Copy link
Owner

jasmcaus commented Jul 7, 2021

Thank you for your suggestions, @stefano-p. You're right, adding the _s leads to greater readability, but not always.
The point of assertion macros is to do their job, providing just the right amount of clarity.

A STR* compared to STR_* is slightly less readable, yes, but it makes sense. I shy away from extreme verbosity (eg STR_EQ) when STREQ gets the point across. Now, this would not make sense for BUF_EQ because this makes more sense that BUFEQ

I'm aware that this results in slight non-uniformity in the codebase, but the point is to have the macros make the most sense without overdoing on the _ chars.

@stefano-p
Copy link
Contributor Author

I think I got your point. If we just consider the CHECK_STROK macro itself, I agree with you that STROK sounds better than STR_OK.
But now I am considering the whole view of this framework. From this standpoint, uniformity is also important. So, in general you choose a pattern and then you want to stick to it without exceptions.
In this case the pattern is CHECK
{object}
{condition}, which basically applies to all macros with the exception to those related to strings. Are you really sure that this is what you want?

@jasmcaus
Copy link
Owner

jasmcaus commented Jul 7, 2021

Yes, the only break from uniformity is in the BUF_* macros. Otherwise, the rest are in the format {CHECK|REQUIRE}_*.
I think it's too early to decide on this at the moment. The goal with Tau is to provide clean and readable assertion macros. Uniformity is encouraged, but not the sole defining factor (of course, this works both ways since pure non-uniformity is a signal of chaos, not what we're trying to optimize for).

Hope this helps.

@jasmcaus
Copy link
Owner

@stefano-p, will you be working on a PR for this?

@stefano-p
Copy link
Contributor Author

@jasmcaus , I was thinking that your point of view makes sense to me, but we did not find full agreement on the naming and you also stated that "it's to early to decide at this moment". Maybe a wiser decision will come later, possibly with feedback coming from other people. So I think that for the time being I can solve this on my side only, by adding a wrapper for the macros in question, without touching the original repo.
In the meanwhile I'll keep reviewing other frameworks, to get the big picture.

@jasmcaus
Copy link
Owner

Actually, I strongly feel that this change should be made now, opposed to later. Tau has very few users currently, so the scope of change will not have a significant impact.

If later there arises a need to change away from SUBSTR, we can take a call on that later. For now, I think we'll go ahead as discussed.

@stefano-p
Copy link
Contributor Author

Okay but I am not sure that I can find the time this week, if it is urgent. Hopefully this weekend.

@jasmcaus
Copy link
Owner

Sure, no hurry

@jasmcaus
Copy link
Owner

Fixed in #9

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

No branches or pull requests

2 participants