Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for recursive extends (adjusted) #24

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

HoldYourWaffle
Copy link

@HoldYourWaffle HoldYourWaffle commented Apr 19, 2019

(supersedes #22)

As mentioned in #22, I need this feature in my project. Sadly @josephfinlayson never made the adjustments you wanted, so I did it.

Changes relative to #22:

  • Added a complete & working test*
  • Undid the irrelevant changes (like package.json)

There are 3 things I'm not 100% sure I did correctly:

  • My test somehow requires an exact whitespace match (as far as I can tell), even though the original extends inherited layout test is almost the exactly the same.
  • @josephfinlayson commented out line 91-96, but I can't figure out why you'd want to do that. Especially since it makes a test fail.
  • Is the commit history clean enough? I used a merge to make sure @josephfinlayson got his credit, would a cherry-pick be better?

@HoldYourWaffle
Copy link
Author

When I tried to use this in my own project it didn't work as I expected (throwing an error that definitely shouldn't be there), currently investigating.

@HoldYourWaffle
Copy link
Author

After a lot of staring I think I found something that could lead to the cause. Keep in mind that I have no idea how reshape works beyond the code of this plugin, so there may be some (big) inaccuracies. I also don't have the code in front of me right now, so the variable names might be slightly inaccurate.

It seems like the tree modification logic (changing just the contents of nodes) is not sufficient to support this behavior. The merged tree is the same before and after the merge (this is something I didn't understand at all), so the already merged blocks are still passed on to the 'next layer'.

I'm way too tired to be doing this right now but I'll be looking at this again tomorrow.

@jescalan
Copy link
Member

Let me know if you want to pair and walk through this whenever you're taking another look!

@josephfinlayson
Copy link

Also happy to pair!

@jescalan
Copy link
Member

@josephfinlayson shoot me an email escalantejeff at gmail and let's set up a time 😁

@HoldYourWaffle
Copy link
Author

I'm going on vacation soon so I won't have any time to look at this until somewhere next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants