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

Contentful Entry Migration functions #3

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

Conversation

elvingm
Copy link
Contributor

@elvingm elvingm commented May 22, 2017

  • Adds createEntry and updateEntry functions
  • Adds migrateEntries (uses updateEntry) and seedEntries (uses createEntry) functions
  • ReadMe update describing Entry migration API

@elvingm elvingm requested review from kylemac and jescalan May 22, 2017 21:58
@elvingm elvingm changed the title Contentful Entry Migration functions [wip] Contentful Entry Migration functions May 22, 2017
@elvingm elvingm changed the title [wip] Contentful Entry Migration functions Contentful Entry Migration functions May 23, 2017
@elvingm elvingm force-pushed the em.entry-create-update branch from 5a38ca1 to 22cf6a1 Compare May 23, 2017 21:14
Copy link
Contributor

@jescalan jescalan left a comment

Choose a reason for hiding this comment

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

Looks great! Just one small comment. This project is shaping up, looking much better. Thanks for the contributions 🙌

seedEntries () {
const entries = fs.readdirSync(this.entriesPath).map((f) =>
require(`${this.entriesPath}/${f}`)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is repeated between the two methods, maybe pull into a common function?

const entries = fs.readdirSync(this.entriesPath).map((f) =>
require(`${this.entriesPath}/${f}`)
)
return Promise.all(flatten(entries).map((e) => this.createEntry(e)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could roll the flatten in as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For flatten do you mean add flatten as part of the exported Client?

Copy link
Contributor

Choose a reason for hiding this comment

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

More like:

seedEntries () {
  return Promise.all(getEntryStructures().map(this.createEntry.bind(this)))
}

getEntryStructures () {
  const entries = fs.readdirSync(this.entriesPath).map((f) =>
    require(`${this.entriesPath}/${f}`)
  )
  return flatten(entries)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. I handled the readdirSync a little differently. Incorporating both then, what do you think of:

  getEntryStructures () {
    return flatten(this.readDataFromPath(this.entriesPath))
  }

  readDataFromPath (path) {
    return fs.readdirSync(path).map((file) => require(`${path}/${file}`))
  }

  migrateContentTypes () {
    const models = this.readDataFromPath(this.modelsPath)
    return Promise.all(models.map((m) => this.updateContentType(m)))
  }

  migrateEntries () {
    return Promise.all(getEntryStructures().map((e) => this.updateEntry(e)))
  }

  seedEntries () {
    return Promise.all(getEntryStructures().map((e) => this.createEntry(e)))
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks great, much cleaner and more DRY 🙌

Maybe just use a path.join rather than the manual string concat in the readDataFromPath method, otherwise this looks perfect

@PaulKiddle
Copy link

Hello! I notice this PR hasn't been touched for a while, any chance of it being completed in the near future?

@kylemac kylemac removed their request for review August 28, 2019 00:40
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