Ecosyste.ms: Timeline
Browse the timeline of events for every public repo on GitHub. Data updated hourly from GH Archive.
eskultety created a review comment on a pull request on containerbuildsystem/cachi2
> I'm not opposed to it, if that's what the maintainers prefer, but I'd like you to think about the toll work that this creates for me to constantly rewrite stuff that seemed to be agreed upon befo...
eskultety created a review comment on a pull request on containerbuildsystem/cachi2
> This way all issues will be detected in one pass. Not bad, this kind of handling is what I've missed in so many tools I have used in my life which only failed at the nearest problem.
eskultety created a review comment on a pull request on containerbuildsystem/cachi2
Because sets have O(1) complexity on lookup which is much faster than searching a list (O(N)) in the loop below.
eskultety created a review comment on a pull request on containerbuildsystem/cachi2
The unfortunate reality here is that it's a nested model, so to IMO do it by the book would mean to effectively validate the lockfile twice - once as a dict from the YAML parser, loop over all arti...
kosciCZ created a review comment on a pull request on containerbuildsystem/cachi2
Pairs is used here as a general concept, not a typing notation. I will change it to reflect L30 better. > nitpick: Do we want this to be a map? I mean most commonly (those I have seen) are alway...
eskultety created a review comment on a pull request on containerbuildsystem/cachi2
Fully agree with @brunoapimentel that all of those but one tests are isolated to lockfile validation which doesn't integrate with any external functionality and so unit tests are sufficient.
eskultety created a review comment on a pull request on containerbuildsystem/cachi2
nitpick: Do we want this to be a map? I mean most commonly (those I have seen) are always string lists. I know that it probably makes it just a bit simpler to get a checksum object out of it, but I...
a-ovchinnikov created a comment on a pull request on containerbuildsystem/cachi2
> We have it for all e2e tests for all package managers Done!
slimreaper35 created a review comment on a pull request on containerbuildsystem/cachi2
Because you are checking if something already exists in the set. That seems a bit off to me. Sets do not contain duplicate values. That's why I raised the question.
kosciCZ created a review comment on a pull request on containerbuildsystem/cachi2
Agree with Erik, it should be rather explicit, we definitely don't want to infer that information, as that could be potentially _a lot_ of code.
kosciCZ created a review comment on a pull request on containerbuildsystem/cachi2
Agree that any unexpected mismatch should fail loud and clear.
kosciCZ created a review comment on a pull request on containerbuildsystem/cachi2
I agree that we can drop the lockfile format related tests, as they are well covered with unit tests. However I'd argue that we should keep those that verify lockfile targets in some way, as those ...
kosciCZ created a review comment on a pull request on containerbuildsystem/cachi2
Good point, will do.
kosciCZ created a review comment on a pull request on containerbuildsystem/cachi2
Thanks. There was a couple of options, but based on previous review comments, this seems to be the preferred option. I'm aware of the downsides that come with it.