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

Update Grid definition Ids #10569

Merged
merged 4 commits into from Sep 21, 2018

Conversation

Projects
None yet
7 participants
@sarjon
Member

sarjon commented Sep 20, 2018

Questions Answers
Branch? develop
Description? Removes _ from grid ids and updates to camel case. Also registers new hooks during installation.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? Behavior does not change in this PR.

This change is Reviewable

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Sep 20, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 20, 2018

Contributor

Once tests pass, I'll merge => 0 risks, only gains :)

@PrestaShop/prestashop-core-developers this pull request allow the hook debugger to display the Grid hooks even if no module register it, and rename it to make the hook name more consistent with others hooks 👍

Contributor

mickaelandrieu commented Sep 20, 2018

Once tests pass, I'll merge => 0 risks, only gains :)

@PrestaShop/prestashop-core-developers this pull request allow the hook debugger to display the Grid hooks even if no module register it, and rename it to make the hook name more consistent with others hooks 👍

@matks matks added the migration label Sep 20, 2018

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Sep 20, 2018

Contributor

wouldn't it be easier to have a special getter that Camelize the getId() value, getHookGridId() for example
this way even if developper forget about the correct syntax it will work fine, and we can keep get getId in (potentially) snake case elsewhere (eg javascript) since it is less important

Contributor

jolelievre commented Sep 20, 2018

wouldn't it be easier to have a special getter that Camelize the getId() value, getHookGridId() for example
this way even if developper forget about the correct syntax it will work fine, and we can keep get getId in (potentially) snake case elsewhere (eg javascript) since it is less important

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Sep 20, 2018

Member

Unit tests are failing 🙁

Member

eternoendless commented Sep 20, 2018

Unit tests are failing 🙁

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Sep 20, 2018

Contributor

Travis crashed, I rerun tests :)

Contributor

PierreRambaud commented Sep 20, 2018

Travis crashed, I rerun tests :)

@sarjon sarjon dismissed stale reviews from PierreRambaud and mickaelandrieu via 8d8511f Sep 21, 2018

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 21, 2018

Member

anybody has any ideas why its failing? :/ i cant understand how these changes may affect tests. 😞

Member

sarjon commented Sep 21, 2018

anybody has any ideas why its failing? :/ i cant understand how these changes may affect tests. 😞

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Sep 21, 2018

Contributor

@sarjon I know. these tests are using hook and module ids, you add new hook, so ids change =)

Contributor

PierreRambaud commented Sep 21, 2018

@sarjon I know. these tests are using hook and module ids, you add new hook, so ids change =)

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 21, 2018

Member

these tests are using hook and module ids

right.. now i understand, ill need to find a way to fix it.

Member

sarjon commented Sep 21, 2018

these tests are using hook and module ids

right.. now i understand, ill need to find a way to fix it.

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Sep 21, 2018

Contributor

@sarjon I can have a check this afternoon if you want?

Contributor

PierreRambaud commented Sep 21, 2018

@sarjon I can have a check this afternoon if you want?

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 21, 2018

Member

@PierreRambaud i've tried to fix it, lets see if it works out. 🤞

Member

sarjon commented Sep 21, 2018

@PierreRambaud i've tried to fix it, lets see if it works out. 🤞

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 21, 2018

Member

all is good. 👍

Member

sarjon commented Sep 21, 2018

all is good. 👍

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 21, 2018

Contributor

Thanks @sarjon and everyone for the reviews 👍

Contributor

mickaelandrieu commented Sep 21, 2018

Thanks @sarjon and everyone for the reviews 👍

@mickaelandrieu mickaelandrieu merged commit d43e37b into PrestaShop:develop Sep 21, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment