NOAA-EMC / NCEPLIBS-bufr

The NCEPLIBS-bufr library contains routines and utilites for working with the WMO BUFR format.
Other
42 stars 19 forks source link

add bort test to urbrw #555

Closed jack-woollen closed 4 months ago

jbathegit commented 5 months ago

@jack-woollen thanks for this, but I have a couple of requests:

  1. test_ufbrw is basically an application code, and from a design and encapsulation standpoint we never want application codes directly accessing COMMON blocks inside of the library. So instead of having a direct link to COMMON /QUIET / IPRT, please delete that line, and then correspondingly change your if(i==1) iprt=2 and if(i/=1) iprt=0 lines to be if(i==1) call openbf(20,'QUIET',2) and if(i/=1) call openbf(20,'QUIET',0). That's how any application code should be accessing IPRT (i.e. indirectly!) in order to turn on and off higher print verbosity.
  2. On a related note, and based on where you've currently positioned your if(i==1) and if(i/=1) lines, the increased verbosity is now in effect for not just the call to ufbint(), but also for ireadmg(), ireadsb(), and closbf(), as well as for openbf() when i==2. As a result, this is generating a ton of unnecessary FT06 test output (about 3700 lines worth for each of the test_ufbrw_4, 8, and d cases!) when all you're really trying to test here is the increased verbosity inside of that one call to ufbint(). So instead, how about just adding your switches directly around that one call to ufbint(), e.g. something like the following which would significant reduce the overall amount of FT06 output:
    if(i==1) call openbf(20,'QUIET',2)
    call ufbint(20,arr,10,255,irt,cond//' POB QOB TOB UOB VOB')
    if(i==1) call openbf(20,'QUIET',0)
jbathegit commented 5 months ago

And BTW, I only happened to see this b/c I just recently submitted a PR of my own. If you have something like this ready to be reviewed, please right-click on the "Reviewers" button and select me along with anyone else you want from the resulting pull-down menu. That way, it automatically alerts us that we've been requested for a code review on that particular PR.

It's also good practice to put at least a cursory description in the main text box of what you did and what issue(s) your PR is related to, though in this case it's fairly simple and obvious so you could just put something like Part of #445

jack-woollen commented 5 months ago

Thanks Jeff. This is a work in progress, not ready to be reviewed. Just made a pull request so it would update the test-coverage as I go. Good advice about not having a common block, I'll fix that. Actually the iprt is meant to hit lines in ufbrw. Best I can do for that is put the on/off switches only around the call to ufbint. And I'll definitly let you know when it needs to be reviewed.

jbathegit commented 5 months ago

Thanks Jeff. This is a work in progress, not ready to be reviewed.

Ahh, OK, and in that case what you could do in the future is just open your PR as a draft, and which in turn would make it clear that it's not yet ready to be looked at. Then, once you are ready for it to be reviewed, just click the green status button to remove the draft status and mark it as ready for review.

Actually the iprt is meant to hit lines in ufbrw. Best I can do for that is put the on/off switches only around the call to ufbint.

I kind of figured that, and which is why I suggested that very same thing (i.e. putting the on/off switches only around the call to ufbint) within the second request of my initial reply :-)

jack-woollen commented 5 months ago

Good point. I don't see where to open a draft pull request. As for extra print, fortunatley, it only needs to have iprt=2 for one trip (not one file) through ufbint/ufbrw, so that's the smallest footprint it will have.

jbathegit commented 4 months ago

This PR was superseded by #567