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

some improvements for migrations and for ZINNIA_MARKDOWN_EXTENSIONS #401

Closed
wants to merge 6 commits into from

Conversation

mavriq
Copy link
Contributor

@mavriq mavriq commented Feb 18, 2015

  1. you can use ZINNIA_ENTRY_BASE_MODEL for determine your own model of Entry. But migration is trying to create a model of AbstractEntry. I tried to fix this problem.
  2. I think such a definition of ImageEntry.image field would be better
  3. I think use the list of modules (or names) while setting options is an better idea than the join this list to one big string, and then split them (for ZINNIA_MARKDOWN_EXTENSIONS)

@mavriq mavriq changed the title some improvements some improvements for migrations and for ZINNIA_MARKDOWN_EXTENSIONS Feb 18, 2015
@mavriq
Copy link
Contributor Author

mavriq commented Feb 18, 2015

sorry. my mistake.
I rushed to publish and correct and not tested parts of the code.
Now try to fix it, and will give results.

@mavriq
Copy link
Contributor Author

mavriq commented Feb 19, 2015

corrected errors
added in demo/ an example of usage ZINNIA_IMAGE_FIELD
this time should work correctly

@Fantomas42
Copy link
Owner

Hello @mavriq

I will give you a little review of your PR, because you worth it with your impressive PR.

Point 1.

Your solution is really smart, but why not using this setting https://docs.djangoproject.com/en/dev/ref/settings/#migration-modules ? I think your implementation save the case where you have removed some fields in the Entry model, but what do you if I want to add more fields ? I certainly miss something...

Point 3.

This seems perfect. 👀 🐅

Point 2.

I have a real dilemma.
I see that you have put a lot of work and probably time in this feature, but the implementation is exactly that I don't want to do in Zinnia. This treat the image field like an exception. This exception is everywhere in the codebase (=> pain to maintain) and this IF is so ugly that my eyes are melting. :)

Note this review is not final.

Thanks for your work !

@mavriq
Copy link
Contributor Author

mavriq commented Feb 25, 2015

Hello, @Fantomas42
Thank you for your feedbak, and sorry for the long silence.

Point 3.
This seems perfect. 👀 🐅

glad to be helpful. maybe, at this stage, possible to take this changes: 87af853 3d66f63

Your solution is really smart, but why not using this setting https://docs.djangoproject.com/en/dev/ref/settings/#migration-modules

hmm.. just django - is my hobby. yet I have no lot of experience. I saw this parameter, but I did not think that it can be used so :)

I see that you have put a lot of work and probably time in this feature, but the implementation is exactly that I don't want to do in Zinnia

now I see, that the way I chose was wrong. work and time that I put is for my experience, and if it way was unsuccessful... now I know how I should not have to do.
now, I think, I know what is was necessary to do, and what is not.
if you would be interested - I will try in the next couple of days to show it.

@mavriq mavriq closed this Feb 25, 2015
@zopieux
Copy link
Contributor

zopieux commented Mar 27, 2015

Oh damn, did not see this PR before I opened mine: #406. I did more or less exactly the changes @mavriq did for the MARKDOWN_EXTENSIONS stuff.

I am glad to find out I am not the only one annoyed with that parameter :-)

@Fantomas42
Copy link
Owner

@mavriq 87af853 has been merged, and your idea of simplification for the Markdown extensions has also been applied.

Thanks for your work !

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