search

rubyatscale / danger-packwerk

Danger plugin for packwerk
MIT License
22 stars 5 forks source link

Support apps in non-root locations #40

Closed jarredhawkins closed 1 year ago

jarredhawkins commented 1 year ago

Targeting a fix for https://github.com/rubyatscale/danger-packwerk/issues/39

Passing in lambda seemed like the most flexible solution here, but the inversion logic ended up getting a little messier than expected... throwing this up for some feedback either way.

jarredhawkins commented 1 year ago

@alexevanczuk thanks for the quick response! Overall, I do like that approach more -- seems intuitive, and the avoiding inversion logic seems like a net win.

Trying a few more tests on my application led me to discovering some errors inside of the DangerPackageTodoYmlChanges#get_violation_diff based on the a similar direct access of git.modified_files, so these changes might get a little more involved that I initially specced here.

I think that transforming them all of these paths immediately makes sense, but then it also pretty quickly runs into the issue of ensuring how to make sure future maintainers don't forget to prefix every operation with the relative path transformation (I'll let you speak to how much of a concern that really is, but seems reasonable to assume we don't want to plaster relative_path_from everywhere).

My brain jumps to adding in some kind of git/file manipulation wrapper class that most of the read/write/git.* operations are pulled into.

Fully open to your thoughts/suggestions here though, since I'm definitely operating without context.

alexevanczuk commented 1 year ago

My brain jumps to adding in some kind of git/file manipulation wrapper class that most of the read/write/git.* operations are pulled into.

That might work @jarredhawkins! Seems like it should allow us to isolate the relative_path_from call to few places.

jarredhawkins commented 1 year ago

@alexevanczuk pushed up some more changes here taking the wrapper approach. It's a little rough and still lacking tests, but managed to get everything functioning with these.

Open to feedback here, otherwise I'll probably write some tests in the coming week(s) if it looks good.

jarredhawkins commented 1 year ago

Let me know when you're ready for another review.

@alexevanczuk long time coming, but finally got around to writing some tests. Should be good for a full review now.

jarredhawkins commented 1 year ago

thanks for the eyes! ended up pulling a few functions into the private module to get around the max 100 statements per-class cop. Happy to undo those and disable the rule for that class if this ends up muddying more than planned.

alexevanczuk commented 1 year ago

Thanks @jarredhawkins ! We can probably disable that cop later but I think it's fine for now!