search

mainmatter / ember-asset-size-action

Comment with the diff for the asset sizes on Pull Request
MIT License
22 stars 15 forks source link

Support working directory and pre-built assets/artifacts #8

Closed NullVoxPopuli closed 1 year ago

NullVoxPopuli commented 4 years ago

Why?

Demo: https://github.com/NullVoxPopuli/emberclear/pull/532

NullVoxPopuli commented 4 years ago

Well, this is unexpected: image from: https://github.com/NullVoxPopuli/emberclear/pull/532/checks?check_run_id=476885661

backspace commented 4 years ago

I should have started a review instead, sorry. I’m still debugging things laboriously via pushes, but there are a couple of other things that need to be changed too. I’ll make a review instead when it’s working.

NullVoxPopuli commented 3 years ago

Rebased

NullVoxPopuli commented 3 years ago

yikes re: node_modules. Why do these have to be check in 🙃

mansona commented 3 years ago

So there is a lot going on in this PR 😳 I know adding the node_modules makes it a bit more bloated but I am actually talking about the commits themselves.

As I mentioned in my recent blog post we should probably only do exactly one thing in a PR.

Do you want to split out the "Support working directory" feature into it's own PR and then we can discuss the other one afterwards?

NullVoxPopuli commented 3 years ago

As I mentioned in my recent blog post we should probably only do exactly one thing in a PR.

I get it, I do, but when you want to use something, it's much much much much harder (and sometimes impossible.. or just not worth the time) to stitch a ton of PRs together so that you can use them. before waiting who knows how long for maintainers to review and get through all the small PRs.

I will not be splitting up this PR -- partially for the reason above, partially because, on my screen, the non-node_modules changes are only 2 pages (and also I don't have enough time, so, for just open-source, non-work, this process isn't worth it to me).

If someone else wants to split it up, have at it 🙃

NullVoxPopuli commented 3 years ago

I just looked through the commits, looks digestible that way <3

NullVoxPopuli commented 3 years ago

Well, I felt motivated / frustrated, so I split some of the main changes that were confusing to this PR: https://github.com/simplabs/ember-asset-size-action/pull/32

btecu commented 3 years ago

Contains some bad commits? image

backspace commented 3 years ago

I think it’s a sad consequence of needing to include node_modules 😞

hellobontempo commented 2 years ago

Hi there! Curious if there was any update on potentially adding working directory support?

NullVoxPopuli commented 2 years ago

sorry, I lost steam and energy on this, feel free to take over. I'll just not have an asset size action, I suppose :see_no_evil:

hellobontempo commented 2 years ago

I completely understand! 😅 Thanks for the prompt response 😄

mansona commented 1 year ago

Hey folks 👋 just to let y'all know there is now working-directory support in master and you can target that directly in your actions until we do our next release which will hopefully be soon (but it's a major so it might take a tiny bit more time)

I'm going to go ahead and close this PR because a) it's out of sync with master quite a lot now for a number of different reasons and b) because it was too big and doing too much it was next to impossible to get merged.

I think my previous comment about splitting this out into multiple PRs is quite poignant considering that was probably the major reason for the delay in getting this reviewed. I understand your point about the difficulty of "stringing PRs" but in reality having any PR that does more than one thing makes it next to impossible for anyone to review, especially if they are busy in either their work or personal lives.

It's also worth noting that the PR that ultimately landed the working-directory support actually uncovered a significant bug in the use of @actions/exec that would have prevented the feature from working. I only noticed that bug because the PR was small and easy to review/test. I can't verify if your implementation actually fixed the bug because I can't even open the files tab because it crashes my chrome 🤷

mansona commented 1 year ago

And as I said in my previous comment, if you want the pre-built assets/artifacts implementation then open a new PR.

If you do open a new PR for the pre-built assets/artifacts implementation then please just implement it in-place with the smallest diff you can. Any refactors that are blockers to merging the feature will probably delay the process, and likely will make it end with the same fate as this PR and never getting merged.