Skip to content

Feature: Implement watch methods for PDO driver#435

Merged
glensc merged 3 commits into
perftools:0.19.xfrom
fengqi:implement-watch
Nov 6, 2021
Merged

Feature: Implement watch methods for PDO driver#435
glensc merged 3 commits into
perftools:0.19.xfrom
fengqi:implement-watch

Conversation

@fengqi

@fengqi fengqi commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

Add saveWatch, getAllWatches, truncateWatches to PdoRepository:

@fengqi fengqi changed the title implement watch implement pdo watch Nov 5, 2021
@glensc

glensc commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

you need to document the breaking change in the PR body.

@fengqi

fengqi commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

you need to document the breaking change in the PR body.

ok

@fengqi

fengqi commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

done

@glensc

glensc commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

done

Well, the breaking change is that the table is no longer configurable via XHGUI_PDO_TABLE environment variable:

        'table' => getenv('XHGUI_PDO_TABLE') ?: 'results',

@fengqi

fengqi commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

done

Well, the breaking change is that the table is no longer configurable via XHGUI_PDO_TABLE environment variable:

        'table' => getenv('XHGUI_PDO_TABLE') ?: 'results',

change the db table name is not a routine operation, and mongo is not configured, so I hard-coded it in the __construct

@glensc

glensc commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

I'm saying that the breaking change I described is not in PR body. it's breaking change for users who have relied on the environment variable.

@fengqi

fengqi commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

I see, this is indeed a problem, let me change it back.
do you have any other suggestions on this?

@glensc

glensc commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

@fengqi I'm saying you need to document it in the pull request body. you can of course restore the breaking change, so then it does not need to be documented.

@fengqi

fengqi commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

i have changed it back
and i will submit other pdo implementation parts later

@glensc

glensc commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

please consider doing atomic commits for further changes:

and if previous commit needs to be improved, git commit --fixup.

I'll squash/fixup myself your commits here before the merge.

Comment thread src/Db/PdoRepository.php Outdated
@fengqi

fengqi commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

please consider doing atomic commits for further changes:

and if previous commit needs to be improved, git commit --fixup.

I'll squash/fixup myself your commits here before the merge.

got it, I pick all commit from the our used internally branch, so the modification was submitted all at once

@glensc

glensc commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

I use git format-patch -1 COMMIT and git am 0001-foo.patch to transfer such commits. or if they are in same git repo (just different branches) can git cherry-pick COMMIT

@fengqi

fengqi commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

did i need to reopen a new pr?

@glensc

glensc commented Nov 5, 2021

Copy link
Copy Markdown
Contributor

@fengqi open new PR for what?

@glensc glensc changed the title implement pdo watch Feature: Implement watch methods for PDO driver Nov 6, 2021
Co-authored-by: fengqi <lyf362345@gmail.com>
@glensc glensc added this to the 0.19.0 milestone Nov 6, 2021
…ches

Co-authored-by: Elan Ruusamäe <glen@delfi.ee>
All methods need the schema, so may as well create it in constructor
@glensc

glensc commented Nov 6, 2021

Copy link
Copy Markdown
Contributor

@glensc glensc merged commit 829febc into perftools:0.19.x Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants