User Details
- User Since
- Jun 9 2015, 11:31 AM (405 w, 6 d)
Fri, Mar 17
We don't use cPerf in Apple's testing, so I am not familiar with this part of the code. Hopefully one of the other reviewer is though?
Fri, Mar 3
LGTM
This LGTM.
Feb 16 2023
LGTM. Thanks!
Sep 12 2022
Jun 22 2022
May 10 2022
Apr 6 2022
Looks good, with the suggested commit message change.
Mar 29 2022
Wonderful! Thanks
Mar 25 2022
Yep, I normally use hashlib as well.
Mar 23 2022
Mar 15 2022
Looks good. thanks!
LGTM
Mar 14 2022
I think this would be safe to land now. Are you still interested in landing it?
Mar 2 2022
Actually, I just realized this has a serious bug. The hash function in python is salted, so results are different between process restarts.
Yep, this looks good then.
LGTM now. Thanks!
Seems fine? LGTM.
Jan 26 2022
LGTM. Thanks!
Dec 7 2021
Dec 3 2021
Wow, v4_graph is complex!
Oh yes! I did have to load the page, then do a refresh before the old parts of the patch went away.
Can you update this patch now that some of the changes have landed?
Nov 29 2021
LGTM
LGTM
Nov 17 2021
I just made a few comments while skimming this.
This still needs to be more targeted.
Nov 15 2021
LGTM. Thanks!
Nov 9 2021
LGTM
Oct 26 2021
LGTM. Thanks!
Oct 25 2021
See the discussion here:
This SGTM. Can you add a note to revisit in a future version of mypy as well.
I have gone through these code areas a lot in the past, I am happy to review this.
As a start, split the non-patch related changes out. Specifically, at least split the changes in: test_matrix_page.py, test_api.py, the renames, the error message updates in views.py, the refactoring in api.py.
Oct 8 2021
Great idea! They can be very slow on large DBs to process.
Sep 16 2021
One real problem that we are having is that the most common submission format that I know if is the git describe output. We are using that everywhere.
Sep 13 2021
Sep 7 2021
Sep 1 2021
Jul 13 2021
Jul 12 2021
Jun 22 2021
In recent versions of mypy, they have a --install-types flag which can do this automatically.
Jan 20 2021
Jan 14 2021
Could you put your analysis of why the noqa is needed in the noqa comments, so that it will be easy to understand from looking at the code?
Oct 2 2020
LGTM. Thanks!
Sep 21 2020
LGTM. Thanks!
Jul 8 2020
Good idea. Thanks!
Jun 6 2020
Good idea!
May 1 2020
Thanks, I didn’t have a system to check my changes on!
Apr 30 2020
Sure, this is fine. Can you add a test?
Sorry, I was reviewing out of order and approved: D79183 which also has a fix for this. I like the test case though, could you commit the test case?
Good catch! Thanks!
Apr 12 2020
Lgtm. Thanks!
Feb 14 2020
Feb 13 2020
LGTM
Just to confirm, you are using git+git not git+https?
Feb 7 2020
Can you remove the commented out old line. Otherwise, LGTM.
In the past there has been rev lock between LNT and LIT, so we try to fetch the most up to date lit we can. The pypi version can be quite out of date.
How is the lit requirement no longer needed? This URL does not work, but the we still require lit to be installed for the system to work. I think this needed to be updated to fetch the latest lit from the GitHub repo.
Feb 3 2020
LGTM
Jan 17 2020
LGMT.
Jan 10 2020
LGTM. Could you maybe update the message something like warning: calling sysctl failed, defaulting to no fma3
Dec 5 2019
I think we need testing on all versions of python we support. I suggest one version on 3 that is recent only. We currently have no python3 users, so maybe just 3.6 and newer?
Dec 4 2019
We should probably use the url parser on these, and check them formally. This is okay for now though.
I don't use Docker. Can someone else review?
Nov 11 2019
Thanks for the comments. LGTM.