Using Castle.Facilities.NHIbernate with NHibernate.Mapping.Attributes #9

Merged
merged 6 commits into from Feb 17, 2013

2 participants

@akurdyukov

I've modified basic example a bit to support both Fluent and vanilla (or NHMA) configuration variants. Also, there's example for NHMA based on your example. Unfortunately, current interface users will need to update their impls a bit.

Please review the code provided. Thanks!

@haf
Owner
haf commented Sep 8, 2012

OK, so what you're saying is that one doesn't need FluentNHibernate anymore, with this pull request?

How come you decided to make the property a Func?`Wouldn't the idiomatic thing do to, be to make it a method that returns Configuration directly?

Also, how come you want to kill my goto ;)? Because there's no way to really limit the range of ints in C#, like GADTs in Haskell would allow me to, it's harder to prove cases outside [0, 2], unless I make it obvious that I terminate the control flow.

@akurdyukov

Yes, with this code user can use FluentNHibernate or vanilla NHIbernate as configuration.

I though that actual building configuration may be long and complicated task, so I moved it to func. I think you're right - I'll make it return NHibernate.Cfg.Configuration directly. My idea was kinda overcomplicating.

I was a little afraid I got http://channel9.msdn.com/niners/Alik/achievements/visualstudio/GotoAchievement this achievement. There's no direct control for index range, but using goto like this is possible hides a problem with new DefaultSessionLifeStyleOption enum item. I modified code a bit to remove goto and unhide the problem. Please take a look at my upcoming commits.

@akurdyukov akurdyukov goto removed
linq simplified
adda225
@akurdyukov

Also there's a thing with tests: now tests require SQL Express with database created to run. Maybe I can change it to SQLite database? This allows to use embedded driver and does not require anything from system. What do you think?

@haf
Owner

I think you are mixing tabs and spaces, making it hard to see the changes -- could you make an addative commit that normalizes spaces?

@haf
Owner
haf commented Sep 9, 2012
Re. SQL - sure, if you want to create a second pull request with that change, I'll merge it.

Just saw your commit, great.

@akurdyukov akurdyukov spaces to tabs convertion
sqlite binaries added to tests (they are installed by nuget install script)
e2eb83b
@akurdyukov

Please take a look at e2eb83b commit, I replaced spaces with tabs

@akurdyukov

I'm feeling a little uncomfortable to ask, but can you please give feedback on my pull request? I want to use nearest facility release in my project.

@haf
Owner

I think it's good and nice. The only oh is changing a method to a property which is a breaking API change, but I don't mind that too much, since it's easy to find anyway.

I'm thrashing a bit in my development environments, currently; migrating to having my primary work on Ubuntu 12.04 and Windows 8, which is why some pull requests, such as this one, takes longer than I'd prefer.

@haf haf merged commit c1d628c into haf:master Feb 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment