Skip to content

Conversation

@dartcafe
Copy link
Collaborator

@dartcafe dartcafe commented Nov 16, 2018

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Works 👍

@nickvergessen
Copy link
Member

Also in theory your migrations are named wrongly.
See nextcloud/server#12275 (comment) for the intended schema.

So in theory your files should be called 0009

@dartcafe
Copy link
Collaborator Author

So in theory your files should be called 0009

OMG. You're right.

@dartcafe
Copy link
Collaborator Author

I can change this, but does this affect the migration system, if users installed the RCs?

@nickvergessen
Copy link
Member

Since RC is git only, you can do it.
Also your migrations look save enough to not cause troubles, because you check if the table exists before adding it again etc.

But you can simply change the file+class names and run the update locally and see if it does 💥

@dartcafe
Copy link
Collaborator Author

dartcafe commented Nov 25, 2018

@nickvergessen thanks for help

  • updated function names
  • removed unused code
  • consolidated migrations

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Since you never released this yet, consolidating is okay. But in theory you should not do this as soon as it was released. You should also never edit a released migration, just add a new one.

And you file names should be 0009 not 000009. But not too important.

But the && stuff needs changing

@dartcafe dartcafe changed the title Added default 0 to' vote_option_id' Consolidate and fix migration Nov 25, 2018
@dartcafe
Copy link
Collaborator Author

And you file names should be 0009 not 000009. But not too important.

@nickvergessen I am a nitpicker. :-) So thanks for the hint. I updated the file names.

@dartcafe
Copy link
Collaborator Author

Since you never released this yet, consolidating is okay. But in theory you should not do this as soon as it was released. You should also never edit a released migration, just add a new one.

@nickvergessen This was indeed because this was never released. I just wanted to make it more handy for the initial use of the migration system.

@dartcafe dartcafe dismissed nickvergessen’s stale review November 25, 2018 18:46

Removed th second table check.

@dartcafe dartcafe merged commit bf59ea3 into master Nov 25, 2018
@dartcafe dartcafe deleted the patch-migration branch November 25, 2018 18:47
@nickvergessen
Copy link
Member

👍🏼

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