Add method to list pending migrations

[cinemast]

This fixes #9

[codecov]

Codecov Report

Merging #10 into master will increase coverage by 2.5%.
The diff coverage is 83.33%.

@@            Coverage Diff            @@
##           master      #10     +/-   ##
=========================================
+ Coverage   82.85%   85.36%   +2.5%     
=========================================
  Files           1        1             
  Lines          70       82     +12     
=========================================
+ Hits           58       70     +12     
  Misses          6        6             
  Partials        6        6

Impacted Files	Coverage Δ
migrator.go	85.36% <83.33%> (+2.5%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f0120b...c640e78. Read the comment docs.

[lopezator]

Hello @cinemast thank you very much for using migrator and for working on this!

TBH, I wanted to keep migrator as thin as possible, but this seems to be a reasonable common feature to have!

I'll take a look on this shortly, but looks pretty nice!

[cinemast]

I was also not sure if this is already bloat. But from an operations perspective this feature calms my nerves.

[lopezator]

I think it's a good idea.

Just one thing, I see that we are replicating the following snippet of code:

count, _ := countApplied(db)

if count > len(m.migrations) {
	return errors.New("migrator: applied migration number on db cannot be greater than the defined migration list")
}

pending := m.migrations[count:len(m.migrations)]

Maybe it would make more sense to tidy up this, or simply export a function that accomplish this task so the users could use it?

Apart from this logic we are just offering printing.

[lopezator]

Something like the Pending() func but returning a pending migration slice, so we can make use of it in Migrate() and also make use of it from outside (e.g. printing pending migrations).

Maybe it could make sense also to merge that func with the existing countApplied() func.

What do you think?

[cinemast]

Yes, of course. I wanted to keep the noise low on changes. But your suggestions make sense to me.

[cinemast]

This would require breaking interface changes because type migration interface would need to be exported and then conflicts with tx based type Transaction struct, which could be renamed to type TransactionTx struct. But I am not sure if this a actually a good idea.

[lopezator]

What you say it's what is being asked at #8 . Any other idea? Leave the PR as is?

I prefer not to make them, but I wouldn't worry too much about breaking changes as we already are at v0.

[lopezator]

We definitevely have a naming issue here, maybe changing the interface name when exporting it would be easier without falling on those problems?

Since the project's name is migrator (maybe the most adequate name for an interface I can think of) according to Effective GO:

https://golang.org/doc/effective_go.html#interface-names

We have to think about alternatives.

I cannot say I love it, but maybe Migrater could work?

https://en.wiktionary.org/wiki/migrater

Thoughts?

[lopezator]

Another option could be (with breaking changes):

Interface export: migration -> Migration
Migration & MigrationNoTx unexport -> migration, migrationNoTx plus wrapping their construction on NewMigration & NewMigrationTx methods.

CC\ @glerchundi

[cinemast]

Hi, I did not want to do heavy changes on your code.
I started a "fork" of your project here: https://github.com/cinemast/dbolve

It also adds change verification of applied migrations, which is also important to me.

[cinemast]

I pushed the interface changes I already had here.

[lopezator]

Cool!

Although I didn't wanted to add Tx suffix to Migration to point out that the "default" behavior is to use transactions and the special cases could be covered by usingNoTx I think this might be the best option.

Let me check the whole code to reason about tests etc, so we can polish things before merging.

Thanks a lot for the work done!

[lopezator]

I'll be off for a few weeks, but will revisit this once I return. 🙂

[lopezator]

Thanks a lot for the job done here, I'ts been very useful in #17

I think that Pending() it's a great idea as you don't have to compare manually the migrations list in code vs the applied ones on database, and you can know how many migrations are pending to apply.

I didn't added Applied() because I think it's very easy to accomplish in userland (and Iif you just want to check and have DB access, a simple SELECT * FROM migrations would suffice) and I want to keep this package thin.

Please, let me know want you think about this @cinemast and if this works for you. There are few other improvements since this also.