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

Added cleaning of data and asynchronous content loading #534

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

Conversation

balupton
Copy link
Member

Horrified this was not merged, thought it was. Then again #339 is still open.

@balupton
Copy link
Member Author

What this provides is #528.

A concern about this, is what about dynamic documents? How will they reference a document which has now had it's content wiped. If the content fetching, rendering, and whatnot was synchronous, it wouldn't be an issue. However it isn't.

content:null
source:null
})
###
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does clean some things up. I see some commented out sections though, is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be debugging.

@RobLoach
Copy link
Contributor

@balupton I'd just suggest 🔒 this. Seems pretty out-dated, and I look at it whenever I go on review streaks and then remember it's out.

Is there another clean updated PR you'd like opened for it?

@balupton
Copy link
Member Author

Yeah. What we need to do is just take the good bits out of this and keep the bad. It's a complex task, but it does need review.

// Output the command we'll be running
console.log(command.join(' '));

// And spawn our command and let us know if any errors occur
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@balupton
Copy link
Member Author

I think for the most part, this can actually get merged in. I think what I've done is just commented out the actual functional (non-cleaning) changes in this, which is simply the clean actions on file and documents, and the cleanFiles method on DocPad. So we could probably merge this in, with all the cleaning, then just not use the actual cleaning methods — but keep them there for benchmarking.

@balupton
Copy link
Member Author

balupton commented Aug 1, 2019

I have become too preoccupied by other things to review, merge, and release this PR.

However, you should have received an invite a while ago to join the DocPad Extras Team, which will give you write access to this repository, so you can merge in the PR.

For an orientation, or if you need any assistance following the semi-automatic release process from CONTRIBUTING.md, then reach out via https://bevry.me/discord/dev

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

Successfully merging this pull request may close these issues.

None yet

3 participants