Bug 1460035 - [RFE] Do not log error if *LibraryLoaded is missing
[RFE] Do not log error if *LibraryLoaded is missing
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: beakerlib (Show other bugs)
25
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Dalibor Pospíšil
Fedora Extras Quality Assurance
: FutureFeature, Patch, Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-08 17:48 EDT by Alois Mahdal
Modified: 2017-06-21 02:30 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Enhancement
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-06-21 02:30:44 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Alois Mahdal 2017-06-08 17:48:05 EDT
Description of problem
======================

LibraryLoaded is currently mandatory (logs ERROR if it's missing--as if it
ran and failed), but in practice it is not always needed (almost never in
my libraries), so it ends up only cluttering the code and documentation.


Proposal
========

Beakerlib should first detect if the handler has been defined; if not,
skip whole part where it's ran.

Alternatively, Beakerlib could automatically define the handler with just
`true`, then the library code could re-define it if needed.
Comment 1 Alois Mahdal 2017-06-08 19:06:23 EDT
PR submitted to GitHub:

    https://github.com/beakerlib/beakerlib/pull/5
Comment 2 Dalibor Pospíšil 2017-06-09 02:10:45 EDT
When the libraries concept was created we agreed on mandatory initialization function not only for initialization but also for sanity checking that it is really a library.
This is documented and it is mandatory part of library's API.
I don't see any advantage of removing that. As the function can be trivial it is also trivial to place it there.
Comment 3 Alois Mahdal 2017-06-09 15:04:36 EDT
(In reply to Dalibor Pospíšil from comment #2)
> When the libraries concept was created we agreed on mandatory initialization
> function not only for initialization but also for sanity checking that it is
> really a library.

Anyway who does 'sanity check that it is really a library'?  Certainly not rlImport (and I don't think it should).  It simply logs error that the initialization handler failed, which is not true because the handler does not exist.

I don't think it's good design to have 2 places to say "i am a Beakerlib library".  We already do it in Makefile and that is much more formal anyway.


> This is documented and it is mandatory part of library's API.
> I don't see any advantage of removing that. As the function can be trivial
> it is also trivial to place it there.

As I said, it clutters code and documentation.   If I have library with 1 public function, I can't because I always have to add LibraryLoaded (which I never need.)

~

Also, another way of looking at it: this concept (mandatory API) does not come in any other languages (at least those I can think of).  What is it so special about beakerlib to justify having it?

Please re-consider this one more time.
Comment 4 Dalibor Pospíšil 2017-06-15 02:55:33 EDT
(In reply to Alois Mahdal from comment #3)
> Anyway who does 'sanity check that it is really a library'?  Certainly not
> rlImport (and I don't think it should).  It simply logs error that the
> initialization handler failed, which is not true because the handler does
> not exist.
rlImport checks (by calling) then <prefix>LibraryLoaded function that the defined prefix matches the function prefix.
> 
> I don't think it's good design to have 2 places to say "i am a Beakerlib
> library".  We already do it in Makefile and that is much more formal anyway.
> 
> Also, another way of looking at it: this concept (mandatory API) does not
> come in any other languages (at least those I can think of).  What is it so
> special about beakerlib to justify having it?
Beakerlib is not a language it is a test framework, or a test library. And as such it has its own rules, this is one of them.

Still I'm missing opinion of other maintainers. My opinion is to reject this issue. If others have opposite opinion we can discuss further.

Moreover a library is not something you write from scratch every day so I think it is not that big deal. Also beaker-wizard should prepare everything for you.
Comment 5 Martin Kyral 2017-06-15 03:57:07 EDT
The purpose of the function is not just to say 'I'm a library, folks' but at the first place to indicate whether the library has been loaded correctly or not. The very minimal check is that the function itself is defined, more sophisticated checks could include checking of the 'real' functions, variables, values, whatsoever.

Thus, I don't support the change.
Comment 6 Dalibor Pospíšil 2017-06-21 02:30:44 EDT
Well we consider this as unjustified RFE, thus closing this bug.

Note You need to log in before you can comment on or make changes to this bug.