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

Make loading data on demand #92

Merged
merged 1 commit into from Nov 18, 2019

Conversation

@manitu-opensource
Copy link

manitu-opensource commented Oct 29, 2019

The patch makes loading the php-iban data from registry.txt on demand.

This avoid loading unnecessary information into php when not using any php-iban* function, especially when using composer and autoloading php-iban.

@globalcitizen

This comment has been minimized.

Copy link
Owner

globalcitizen commented Oct 29, 2019

I want to see code measuring timings as evidence this has any positive effect before merging.

Timings should preferably include:

  • mainline code vs. proposed code
  • OO library vs. procedural library
  • summary timings for 0x 1x 10x 100x 1000x iterations for each major library function

Thanks

@manitu-opensource

This comment has been minimized.

Copy link
Author

manitu-opensource commented Oct 29, 2019

Sorry but... when using .e.g iban_to_machine_format() it is NOT needed to load the registry.txt, but having the function available you have to include the whole php-iban.php (which auto-loads the registry.txt).

Why?

Our simple patch affects 3 (!) lines of codes without any negative impact (most of the functionality is already there, YOU did write it :-) ). Just prevent loading the registry.txt at including the lib, instead when using. Not less, not more.

To your question: It does not save on each function call, it saves when including the php-iban lib while not using it. Just mathematics!

@globalcitizen

This comment has been minimized.

Copy link
Owner

globalcitizen commented Nov 18, 2019

Yes, it works. I am unconvinced it has any practical impact on modern systems, but it is OK to merge. You should work on your communications though, it is sort of pathetic to heckle open source maintainers with that sort of attitude.

@globalcitizen globalcitizen reopened this Nov 18, 2019
@globalcitizen globalcitizen merged commit c2e29dc into globalcitizen:master Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.