search

getappmap / appmap-agent-js

This project is deprecated. Please use https://github.com/getappmap/appmap-node/ to record Node.js applications.
Other
27 stars 8 forks source link

fixes related to dog fooding #214

Closed lachrist closed 1 year ago

lachrist commented 1 year ago

One large refactoring: 0b43e68334d180059a92c9f2339850f114ba9e67 and a bunch of fixes:

dustinbyrne commented 1 year ago

What else is required here before this PR is considered done? I worry that the amount of changes here are already quite extensive with lots of refactors, fixes and features. Are these all linked to a single issue?

lachrist commented 1 year ago

What else is required here before this PR is considered done? I worry that the amount of changes here are already quite extensive with lots of refactors, fixes and features. Are these all linked to a single issue?

@dustinbyrne It is ready now. Sorry for the delay. I updated the description.

dustinbyrne commented 1 year ago

I'm not sure how to review this. Can this be split into separate pull requests?

lachrist commented 1 year ago

I'm not sure how to review this. Can this be split into separate pull requests?

@dustinbyrne Each commit is independent and should build so they could potentially each be in their own PR. But doing the split and making sure that they can actually be released separately is going to be some work which I'm not sure I will be able to do before I come back.

dustinbyrne commented 1 year ago

The issue is that this is 8 issues across ~6500 lines of code in a single pull request. It becomes difficult to split up the responsibility of reviewing these fixes when everything resides within a single pull request.

kgilpin commented 1 year ago

Right - 1 PR per issue would be the expectation. If one PR depends on the merge of a previous one, that's OK. GitHub will adjust the base branch of the PR as the merges occur.

On Mon, May 8, 2023 at 10:30 AM Dustin Byrne @.***> wrote:

The issue is that this is 8 issues across ~6500 lines of code in a single pull request. It becomes difficult to split up the responsibility of reviewing these fixes when everything resides within a single pull request.

— Reply to this email directly, view it on GitHub https://github.com/getappmap/appmap-agent-js/pull/214#issuecomment-1538460228, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAVC662CPRKR77LWNAQQO3XFD7Q3ANCNFSM6AAAAAAW5DKNJQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

appland-release commented 1 year ago

:tada: This PR is included in version 13.9.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: