Pr2

[afragen]

I see you've gone back to trying to load the class from within the class file. This only works in your example plugin and doesn't result in the javascript file being loaded when the class is loaded via an autoloader. The reason your test plugin works is that the require runs if the plugin is active and runs on every page load or every admin page load, not quite certain. With autoloaders this won't happen.

That's the reason I've switched around the instantiation to require

add_action( 'admin_init', array( PAnD::instance(), 'init' ) );

is than here the javascript is loaded. It took me a while to see this issue.

I see our 2 IDE's are fighting with WP Coding Guidelines regarding spacing. I'm using PhpStorm with the WordPress guidelines installed. https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress-Core/ruleset.xml#L105-L155

There's really no reason to call is_admin_notice_active() as a static and doing so will make the function untestable in the future.

Does this make sense?

[afragen]

And there's this about variable alignment.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#indentation

[w3guy]

I agree with your first sentiment. Will get this reverted.

For the second, we can always make assertion with a static method should we incorporate test. No?

Regarding the code standard, I kinda prefer psr2 code style. Will revert my editor to WordPress coding standard for this project.

On Sunday, 7 August 2016, Andy Fragen notifications@github.com wrote:

I see you've gone back to trying to load the class from within the class file. This only works in your example plugin and doesn't result in the javascript file being loaded when the class is loaded via an autoloader. The reason your test plugin works is that the require runs if the plugin is active and runs on every page load or every admin page load, not quite certain. With autoloaders this won't happen.

That's the reason I've switched around the instantiation to require

add_action( 'admin_init', array( PAnD::instance(), 'init' ) );

is than here the javascript is loaded. It took me a while to see this issue.

I see our 2 IDE's are fighting with WP Coding Guidelines regarding spacing. I'm using PhpStorm with the WordPress guidelines installed. https://github.com/WordPress-Coding-Standards/WordPress- Coding-Standards/blob/develop/WordPress-Core/ruleset.xml#L105-L155

There's really no reason to call is_admin_notice_active() as a static and doing so will make the function untestable in the future.

Does this make sense?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/pull/3#issuecomment-238091645, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8uc8IewNLU-7W2-yxtxKMqNlTvC1vSks5qdgWqgaJpZM4JehdV .

[afragen]

For the second, we can always make assertion with a static method should we incorporate test. No?

I honestly don't know. Creating unit tests is one of the things on my learning @TODO list. 😉 http://stackoverflow.com/questions/5961023/unit-testing-and-static-methods I try not to make my life more difficult in the future and avoiding statics for potentially testable code is a way to do this.

I agree with your first sentiment. Will get this reverted.

You don't need to revert, just use my PR. Without this change and using the same spacing guidelines I'm using, or trying to use, results in no changes from the initial PR. Except for your .gitignore.

[w3guy]

Am not new to unit test evident in my OmniPay drivers public repository.

Static method is a pain only when you fail to set up its dependency in Phpunit setup method.

Changing to static method is the only ch she that I intend to make to your PR

On Sunday, 7 August 2016, Andy Fragen notifications@github.com wrote:

For the second, we can always make assertion with a static method should we incorporate test. No?

I honestly don't know. Creating unit tests is one of the things on my learning @TODO https://github.com/TODO list. 😉 http://stackoverflow.com/questions/5961023/unit-testing-and-static-methods I try not to make my life more difficult in the future and avoiding statics for potentially testable code is a way to do this.

I agree with your first sentiment. Will get this reverted.

You don't need to revert, just use my PR. Without this change and using the same spacing guidelines I'm using, or trying to use, results in no changes from the initial PR. Except for your .gitignore.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/pull/3#issuecomment-238092523, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8ucztDyWn4Up88XPlSZlTutgEVqVLlks5qdgkvgaJpZM4JehdV .

[afragen]

Would you like me to do it?

[w3guy]

Not sure we will be needing it. Remove it.

Should we tag this version 2.0 or 1.2?

On Sunday, 7 August 2016, Andy Fragen notifications@github.com wrote:

Not sure we'll need the instance method then. Do you want me to remove or leave it?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/pull/3#issuecomment-238093882, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8ucw7bxbBsd09GC_dBKrcaoN8VJoefks5qdg6RgaJpZM4JehdV .

[afragen]

We will need instance for

add_action( 'admin_init', array( PAnD::instance(), 'init' ) );

Should we tag this version 2.0 or 1.2?

I would go with 1.0 or 2.0 depending upon how widespread you think this project has been in use. LMK and I'll adjust the PR

[w3guy]

Why not we make init a static method so we end up with below

add_action( 'admin_init', array( 'PAnD', 'init' ) );

On Sunday, 7 August 2016, Andy Fragen notifications@github.com wrote:

We will need instance for

add_action( 'admin_init', array( PAnD::instance(), 'init' ) );

Should we tag this version 2.0 or 1.2? I would go with 1.0 or 2.0 depending upon how widespread you think this project has been in use. LMK and I'll adjust the PR

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/pull/3#issuecomment-238094707, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8uc4akUHUzG5ChF6cCMJbFyvKWbWAsks5qdhG-gaJpZM4JehdV .

[afragen]

That would require a change of init() to not use $this as well. Are we changing things now for the sake of changing them or does this make the use easier?

The user is really going to copy/paste the hook from the readme anyway.

[w3guy]

Am all for making things easier for users and making using the library as expressive and easy as possible.

Am sure it is doable.

Your thought.

On Sun, Aug 7, 2016 at 6:23 PM, Andy Fragen notifications@github.com wrote:

That would require a change of init() to not use $this as well. Are we changing things now for the sake of changing them or does this make the use easier?

The user is really going to copy/paste the hook from the readme anyway.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/pull/3#issuecomment-238095423, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8uc-RGCGD55GywD6SaWjeHJpZXuSdXks5qdhSegaJpZM4JehdV .

[w3guy]

And by the way, thanks for all your hardwork and time.

On Sun, Aug 7, 2016 at 6:33 PM, Agbonghama Collins collizo4sky@gmail.com wrote:

Am all for making things easier for users and making using the library as expressive and easy as possible.

Am sure it is doable.

Your thought.

On Sun, Aug 7, 2016 at 6:23 PM, Andy Fragen notifications@github.com wrote:

That would require a change of init() to not use $this as well. Are we changing things now for the sake of changing them or does this make the use easier?

The user is really going to copy/paste the hook from the readme anyway.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/pull/3#issuecomment-238095423, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8uc-RGCGD55GywD6SaWjeHJpZXuSdXks5qdhSegaJpZM4JehdV .

[afragen]

I don't really think it makes a difference. The users of this library will know what it means and I don't think there is any overhead difference. It's stylistic and it's your repo. Anything's doable.

Let me know how you want it and what version number to use.

[w3guy]

I will prefer the init method is static.

Please tag it as 1.1

I can see you are already using it in your github updater plugin. :smile:

On Sun, Aug 7, 2016 at 6:36 PM, Andy Fragen notifications@github.com wrote:

I don't really think it makes a difference. The users of this library will know what it means and I don't think there is any overhead difference. It's stylistic and it's your repo. Anything's doable.

Let me know how you want it and what version number to use.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/pull/3#issuecomment-238096222, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8uc8H-IH4x0MPFPChTHLepuxzsFmX7ks5qdheSgaJpZM4JehdV .

[afragen]

Done.

That version of GitHub Updater hasn't been released. 😉 Still want 1.1?

[afragen]

BTW, to avoid PHP notices every method will end up being static if we remove the singleton. Do you wish to proceed with it?

[afragen]

My preference would be to use statics where they make sense, not just to use them. I think we should revert back to using the singleton and non-static loading.

I think the code is cleaner.

[w3guy]

I think it perfectly make sense to use static methods in this case.

This is a small library with no serious initialization being done when class is instantiated.

Not only you, but a lot of Dev I come across always try to avoid static methods as much as possible.

This is a scenario where it perfectly make sense.

On Sunday, 7 August 2016, Andy Fragen notifications@github.com wrote:

My preference would be to use statics where they make sense, not just to use them. I think we should revert back to using the singleton and non-static loading.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/pull/3#issuecomment-238097649, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8uczT0RJmMw2HwXhe-8KSd3Od8WP8mks5qdh0CgaJpZM4JehdV .

[w3guy]

Yes please. Make all methods static or better still, to not make all the methods static, how about we add the

add_action( 'admin_init', array( PAnD::instance(), 'init' ) );

Inside a function in the class so that calling say

PAnD::init()

Registers the scripts and all other things in

array( PAnD::instance(), 'init' )

By so doing, we reduce the code for botstrapping the library.

On Sunday, 7 August 2016, Agbonghama Collins collizo4sky@gmail.com wrote:

I think it perfectly make sense to use static methods in this case.

This is a small library with no serious initialization being done when class is instantiated.

Not only you, but a lot of Dev I come across always try to avoid static methods as much as possible.

This is a scenario where it perfectly make sense.

On Sunday, 7 August 2016, Andy Fragen <notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

My preference would be to use statics where they make sense, not just to use them. I think we should revert back to using the singleton and non-static loading.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/pull/3#issuecomment-238097649, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8uczT0RJmMw2HwXhe-8KSd3Od8WP8mks5qdh0CgaJpZM4JehdV .

[afragen]

OK, my PR now completely static, no PHP errors and tagged version 1.1.0

add_action( 'admin_init', array( PAnD::instance(), 'init' ) );

Inside a function in the class so that calling say

PAnD::init()

Registers the scripts and all other things in

array( PAnD::instance(), 'init' )

By so doing, we reduce the code for botstrapping the library.

I don't think we can do this as the initial problem of loading outside of the class is needed. It also adds a lot of complexity.

I think we should either stick with all static or add singleton back.

PR now stands as all static, but I can revert.

[w3guy]

All static is fine. Let me review now.

On Sun, Aug 7, 2016 at 7:12 PM, Andy Fragen notifications@github.com wrote:

OK, my PR not completely static, no PHP errors and tagged version 1.1.0

add_action( 'admin_init', array( PAnD::instance(), 'init' ) );

Inside a function in the class so that calling say

PAnD::init()

Registers the scripts and all other things in

array( PAnD::instance(), 'init' )

By so doing, we reduce the code for botstrapping the library.

I don't think we can do this as the initial problem of loading outside of the class is needed. It also adds a lot of complexity.

I think we should either stick with all static or add singleton back.

PR now stands as all static, but I can revert.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/pull/3#issuecomment-238098400, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8uc0JGKiKx-tTRaso0OPHPprwi_Ijwks5qdiAfgaJpZM4JehdV .

[w3guy]

Thanks for all the hardwork. 1.1.0 is now live.