[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Patch for making fedabipkgdiff support --dso-only option



Hello Chenxiong,

The patch looks great and I have applied to master, thanks!

I have just made some slight wording/formatting adjustments:

> 	* tests/runtestfedabipkgdiff.py.in (fedabipkgdiff_mod): Fix a typo
> 	in initializing this global variable. (test_data_dir): New
> 	global variable, that is used to reference
> 	tests/data/test-fedabipkgdiff/.
> 	(RunAbipkgdiffTest.{test_all_success, test_partial_failure}):
> 	Fix typo.  (Mockglobalconfig.{koji_topdir, dso_only}): New data
> 	members.  (GetPackageLatestBuildTest.{test_get_latest_one,
> 	test_cannot_find_a_latest_build_with_invalid_distro,
> 	test_succeed_to_download_a_rpm, test_failed_to_download_a_rpm}):
> 	Fix typo.  (BrewListRPMsTest.test_select_specific_rpms): Fix
> 	typo.  (RunAbipkgdiffWithDSOOnlyOptionTest): New test case
> 	class.

I fixed this so that it has a proper formatting.  That is, referencing
function names (in parenthesis) should be at the beginning of the line.
There must have been some copy/paste issue.  Note that in Emacs, there
is a ChangeLog (M-x change-log-mode) that helps greatly to have
ChangeLog correctly formatted.  Well, Emacs is just great, I love it :-)

> +  * ``--dso-only``
> +
> +    Compare ABI of shared libraries only. By default, to compare ABI of all
> +    binaries within a RPM.
> +

I amended this slightly, so it now reads like:

  * ``--dso-only``

    Compares the ABI of shared libraries only.  If this option is not
    provided, the tool compares the ABI of all ELF binaries found in
    the packages.

>      parser.add_argument(
> +        '--dso-only',
> +        required=False,
> +        action='store_true',
> +        dest='dso_only',
> +        help='Compare the ABI of shared libraries only. By default, ABI of all'
> +             ' binaries are compared.')

Likewise, I made some slight wording adjustments here.  This part now
reads:

    parser.add_argument(
        '--dso-only',
        required=False,
        action='store_true',
        dest='dso_only',
        help='Compare the ABI of shared libraries only.  If this option is not '
             'provided, the tool compares the ABI of all ELF binaries.')

Thank you again for the patch!

Cheers,

-- 
		Dodji