DBIx::SB::Record.pm aliases cleanup and docs

RT Developers
1

Hello.
Attached patch add missing aliases and some words that such aliases exist.

Best regards, Ruslan.

2

Forgot about attachment :slight_smile:

Ruslan U. Zakirov wrote:

record_naming_cleanup.patch (4.24 KB)

3

Forgot about attachment :slight_smile:

Ruslan U. Zakirov wrote:

Hello.
Attached patch add missing aliases and some words that such aliases exist.

What do you think about switching to use miyagawa’s capitalization.pm
for this function instead?

4

Jesse Vincent wrote:

Forgot about attachment :slight_smile:

Ruslan U. Zakirov wrote:

Hello.
Attached patch add missing aliases and some words that such aliases exist.

What do you think about switching to use miyagawa’s capitalization.pm
for this function instead?

First, I had some time to try it but with no success. I didn’t find way
to drop capitalization in current module from module itself. This module
is for external users, that is reason why pragma exists at all. I’ll try
it again under debugger, may be today tonight, just to understand why it
doesn’t work in such way.

Second, considering that I don’t like all this jumping around method
naming, I see several ways to go.

  1. Obsolete lower case naming for 2-3 releases with warning and then
    drop it. Suggest capitalization.pm for users if they used lower case
    variant and don’t want rewrite.

  2. Maintain aliases by hand. We even can write .t file that check that
    all functions has alias. This .t would help to maintain and check that
    you didn’t forget to define alias for new sub.

I like both solutions. The worth solution is to implement
cpatilazation.pm into DBIx::SB because it slows down DBIx::SB startup, a
little, but slowdown and that’s for user to decide if he want lower case
methods and want to pay some CPU cycles for it.

Third, this module/pragma has a little bug and doesn’t work well with
subs like _Accessible, because of using ‘lcfirst’ on return code path
while converting name from upper to lower case. It requires some fixing.

This is what I think. DBIx::SB requires consistent, stable and
documented API, that’s all.

Regards, Ruslan.

5

Hello, Jesse, Tatsuhiko and all.

I’ve added maintainer of the capitalization.pm to notify about bug in
his module that is described in the bottom of this mail.

Ruslan Zakirov wrote:

Jesse Vincent wrote:

Hello.
Attached patch add missing aliases and some words that such aliases
exist.

What do you think about switching to use miyagawa’s capitalization.pm
for this function instead?

First, I had some time to try it but with no success. I didn’t find way
to drop capitalization in current module from module itself. This module
is for external users, that is reason why pragma exists at all. I’ll try
it again under debugger, may be today tonight, just to understand why it
doesn’t work in such way.
I’ve tried again and have found that capitalization.pm works from module
itself only if you put it usage in the bottom of the module after all
sub declarations.

Second, considering that I don’t like all this jumping around method
naming, I see several ways to go.

  1. Obsolete lower case naming for 2-3 releases with warning and then
    drop it. Suggest capitalization.pm for users if they used lower case
    variant and don’t want rewrite.

  2. Maintain aliases by hand. We even can write .t file that check that
    all functions has alias. This .t would help to maintain and check that
    you didn’t forget to define alias for new sub.

I like both solutions. The worth solution is to implement
cpatilazation.pm into DBIx::SB because it slows down DBIx::SB startup, a
little, but slowdown and that’s for user to decide if he want lower case
methods and want to pay some CPU cycles for it.

Third, this module/pragma has a little bug and doesn’t work well with
subs like _Accessible, because of using ‘lcfirst’ on return code path
while converting name from upper to lower case. It requires some fixing.
Yes, it also confirmed and should be fixed. Examples from debug log:
method ‘LoadByCol’ at …/capitalization.pm line 23.
alias ‘load_by_col’ at …/capitalization.pm line 25.
method ‘_Set’ at …/capitalization.pm line 23.
alias ‘_Set’ at …/capitalization.pm line 25.
method ‘_PrimaryKey’ at …
alias ‘_Primary_key’ at …

Jesse, do you have any comments and wishes?

Regards, Ruslan.

6

Tatsuhiko Miyagawa wrote:

  Hello, Jesse, Tatsuhiko and all.

I’ve added maintainer of the capitalization.pm to notify about bug in
his module that is described in the bottom of this mail.

Oh, I’ve never thought someone actually uses this pragma! :slight_smile:
:slight_smile:

First, I had some time to try it but with no success. I didn’t find way
to drop capitalization in current module from module itself. This module
is for external users, that is reason why pragma exists at all. I’ll try
it again under debugger, may be today tonight, just to understand why it
doesn’t work in such way.

I’ve tried again and have found that capitalization.pm works from module
itself only if you put it usage in the bottom of the module after all
sub declarations.

Is allowing “no capitalization” to no-capitalize functions/methods of
the caller(0) package considered useful?
DBIx::SB provide both variants of API with handwriten aliases. We tried
to use your pragma to don’t maintain aliases.

.

Yes, it also confirmed and should be fixed. Examples from debug log:
method ‘LoadByCol’ at …/capitalization.pm line 23.
alias ‘load_by_col’ at …/capitalization.pm line 25.
method ‘_Set’ at …/capitalization.pm line 23.
alias ‘_Set’ at …/capitalization.pm line 25.
method ‘_PrimaryKey’ at …
alias ‘_Primary_key’ at …

Do you want _primary_key here?
Yes. I’ve wrote fix for this. Attached.

Another problem I see is methods with upper case sequences in name, like
special subs(AUTOLOAD, DESTROY) or normal methods like ACL, ID. It’s not
showstopper for DBIx::SB, but “bug”/“undocumented feature” in the pragma.

Regards, Ruslan.

capitalization_pm_fix.pm (1.26 KB)

7

Tatsuhiko Miyagawa wrote:> On 5/6/05, Ruslan U. Zakirov Ruslan.Zakirov@miet.ru wrote:

Do you want _primary_key here?

Yes. I’ve wrote fix for this. Attached.

Thanks, applied.

Another problem I see is methods with upper case sequences in name, like
special subs(AUTOLOAD, DESTROY) or normal methods like ACL, ID. It’s not
showstopper for DBIx::SB, but “bug”/“undocumented feature” in the pragma.

Patches (incl. documentation) welcome :slight_smile:

Attached patch that fix all corner cases I see and adds docs. Applies
against clean 0.01.

Also added note about case when user wants add nocap aliases in the
caller(0) package. I think that this note would be enough and don’t
think that this case should be fixed.

Regards, Ruslan.

capitalization_pm_fix.pm (2.09 KB)

8

Jesse Vincent wrote:

Forgot about attachment :slight_smile:

Ruslan U. Zakirov wrote:

Hello.
Attached patch add missing aliases and some words that such aliases exist.

What do you think about switching to use miyagawa’s capitalization.pm
for this function instead?
Hello, Jesse.

Attach two solutions and three patches. Just for fun! :slight_smile:

  1. via capitalization.pm 0.03
    nocap_via_pragma.patch
    This patch has docs that should be added in any case.

  2. via scrip made aliases
    Wrote small script(fix_aliases.pl) that reads code from STDIN and output
    code with aliases to STDOUT.

cat SearchBuilder.pm | perl fix_aliases.pl >tmp && mv tmp
SearchBuilder.pm

  1. test file that check if all subs has aliases
    nocap_test.patch
    Can be used with both solutions to check API consistency.

And last prefered solution: deprecate all that lower case alias crap and
suggest to use capitalization.pm 0.03 instead :slight_smile:

Choose any :wink:

Regards, Ruslan.

nocap_via_pragma.patch (5.73 KB)

fix_aliases.pl (744 Bytes)

nocap_test.patch (2.49 KB)

9

Attach two solutions and three patches. Just for fun! :slight_smile:

  1. via capitalization.pm 0.03
    nocap_via_pragma.patch
    This patch has docs that should be added in any case.

What do you think of doing this one, but with a “soft requirement” – so
that if the user doesn’t have capitalization.pm, the module still runs,
just inly in “CamelCase” mode?

10

Jesse Vincent wrote:

Attach two solutions and three patches. Just for fun! :slight_smile:

  1. via capitalization.pm 0.03
    nocap_via_pragma.patch
    This patch has docs that should be added in any case.

What do you think of doing this one, but with a “soft requirement” – so
that if the user doesn’t have capitalization.pm, the module still runs,
just inly in “CamelCase” mode?

No problems, here it is. Added feature to Makefile.PL, but looks like it
works nice only when auto_install is turned on :frowning:

If you want to add tests for nocap API then I can send you patch.

Regards, Ruslan.

nocap_via_pragma.patch (6.06 KB)

11

Ruslan U. Zakirov wrote:

Jesse Vincent wrote:

Attach two solutions and three patches. Just for fun! :slight_smile:

  1. via capitalization.pm 0.03
    nocap_via_pragma.patch
    This patch has docs that should be added in any case.

What do you think of doing this one, but with a “soft requirement” – so
that if the user doesn’t have capitalization.pm, the module still runs,
just inly in “CamelCase” mode?

No problems, here it is. Added feature to Makefile.PL, but looks like it
works nice only when auto_install is turned on :frowning:

Thanks. Applied

I turned on auto_install in the Makefile.PL. Also, I reenabled a core
mapping of Id → id, since that’s a pretty common idiom, even when
you’re using InterCapped style for the rest of the code.

If you want to add tests for nocap API then I can send you patch.

It would not be a bad thing.

signature.asc (189 Bytes)

12

Jesse Vincent wrote:

Ruslan U. Zakirov wrote:

Jesse Vincent wrote:

Attach two solutions and three patches. Just for fun! :slight_smile:

  1. via capitalization.pm 0.03
    nocap_via_pragma.patch
    This patch has docs that should be added in any case.

What do you think of doing this one, but with a “soft requirement” – so
that if the user doesn’t have capitalization.pm, the module still runs,
just inly in “CamelCase” mode?

No problems, here it is. Added feature to Makefile.PL, but looks like it
works nice only when auto_install is turned on :frowning:

Thanks. Applied

I turned on auto_install in the Makefile.PL. Also, I reenabled a core
mapping of Id → id, since that’s a pretty common idiom, even when
you’re using InterCapped style for the rest of the code.

If you want to add tests for nocap API then I can send you patch.

It would not be a bad thing.

Attached.

Regards, Ruslan.

nocap_test.patch (2.68 KB)

13

Jesse Vincent wrote:

Ruslan U. Zakirov wrote:

Jesse Vincent wrote:

Attach two solutions and three patches. Just for fun! :slight_smile:

  1. via capitalization.pm 0.03
    nocap_via_pragma.patch
    This patch has docs that should be added in any case.

What do you think of doing this one, but with a “soft requirement” – so
that if the user doesn’t have capitalization.pm, the module still runs,
just inly in “CamelCase” mode?

No problems, here it is. Added feature to Makefile.PL, but looks like it
works nice only when auto_install is turned on :frowning:

Thanks. Applied

I turned on auto_install in the Makefile.PL. Also, I reenabled a core
mapping of Id → id, since that’s a pretty common idiom, even when
you’re using InterCapped style for the rest of the code.
You forget to add ‘inc/Module/Install/AutoInstall.pm’ file to repo :frowning:

14

If you want to add tests for nocap API then I can send you patch.

It would not be a bad thing.

Attached.

Applied.

15

You forget to add ‘inc/Module/Install/AutoInstall.pm’ file to repo :frowning:

Fixed.