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

Fix/sort entries #125

Merged
merged 6 commits into from
Jun 26, 2017
Merged

Fix/sort entries #125

merged 6 commits into from
Jun 26, 2017

Conversation

Khaledgarbaya
Copy link
Contributor

Reorders them so that entries which are linked from other entries always
come first in the order. This ensures that when we publish entries, we
are not publishing entries which contain links to other entries which haven't been published yet.

With this, we can remove the workaround loop in contentful-import

reorders them so that entries which are linked from other entries always
come first in the order. This ensures that when we publish entries, we
are not publishing entries which contain links to other entries which haven't been published yet.
@zcei
Copy link
Contributor

zcei commented Jun 26, 2017

How does this work with cyclic dependencies?

Also two side notes:

  1. maybe move lib/get/sort-entries.js to lib/utils/sort-entries.js now that both get & push depend on it
  2. How is this line ever reached? 🤔

Copy link
Contributor

@axe312ger axe312ger left a comment

Choose a reason for hiding this comment

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

Found some issues, fixed it. @Khaledgarbaya it is your turn for review.

Copy link
Contributor Author

@Khaledgarbaya Khaledgarbaya left a comment

Choose a reason for hiding this comment

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

LGTM

@axe312ger
Copy link
Contributor

axe312ger commented Jun 26, 2017

@zcei

How does this work with cyclic dependencies?

it doesn't 😅 but this is not fixable via sorting, we need another solution for circular references.

  1. maybe move lib/get/sort-entries.js to lib/utils/sort-entries.js now that both get & push depend on it

good point

  1. How is this line ever reached? 🤔

correct, never. So I fixed it.

@Khaledgarbaya Khaledgarbaya merged commit 836bbb3 into master Jun 26, 2017
@Khaledgarbaya Khaledgarbaya deleted the fix/sort-entries branch June 26, 2017 15:19
@axe312ger axe312ger restored the fix/sort-entries branch July 11, 2017 08:02
@axe312ger axe312ger deleted the fix/sort-entries branch July 19, 2017 08:04
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.

None yet

3 participants