OK. I can submit a new patch that uses an overload instead of the config setting, and one for the the documentation. The method I mentioned in in DriverBase is private, so no API concerns. I'll send a patch for that too, it's just one line. You can take a look.
The only problem now is my Jira account (mentioned in a separate post). Sorry for the inconvenience on that; don't know what happened. Thanks for the help. BTW, just wanted to say thanks for NH too! NHibernate is a huge part of my work these days. I work for a consulting firm (we build custom business app) and we now have four projects that rely on it for persistence. I have now taught many developers how to use it. I'm a huge believer. We're very excited about 3.0, the new Linq provider, etc. Chris On Mar 5, 12:27 am, Fabio Maulo <[email protected]> wrote: > for time-out I think is better to use an overload with the specific time-out > only for SchemaExport. > for the other matter of point 1 I would see what you are proposing and then > try to understand why. > > There are times where we prefer don't change the signature of a public > method and avoid a breaking change, take care. > The most big breaking change was 1.2->2.0 (renaming of some classes, methods > and namespace); if the reason of breaking change is not so strong we prefer > avoid it. > > About the documentation: thanks, a patch is always welcome > > 2010/3/5 Chris Collier <[email protected]> > > > > > > > Hi Fabio, > > > There are three parts, two fixes and some low priority cleanups I > > could optionally do while in the code: > > > 1. Currently, the command_timeout configuration setting has no effect > > when using SchemaExport. The patch I submitted to Jira: > >http://216.121.112.228/browse/NH-2126, fixes this. I think > > SchemaExport should look at this setting, because otherwise there is > > no way to set commandTimeout on the IDbCommands. AFAIK, it has to be > > set on the IDbCommand object directly, which there is no access to > > outside SchemaExport. Users may or may not want a different timeout > > setting for DB creation than for queries, but this at least gives them > > the option of setting it at all for DB creation. I guess another > > option would be to expose another public Execute method that takes a > > caller-supplied IDbCommand rather than an IDbConnection. > > > We need it in our project because we're abusing <database-object> in > > HBMs to execute long-running ETL stored procs to populate our database > > at the end of the DB creation. One of the ETL operations takes > > approx. 45 seconds, which is longer than the 30-second default > > commandTimeout value. > > > 2. I don't know if the same problem exists in SchemaUpdate, but I can > > submit a patch for that if so, since I'm now familiar with that part > > of the code. > > > 3. Cleanups: First, the two public Execute(bool, bool, bool, > > IDbCommand, TexWriter) methods in SchemaExport have comments. But the > > two public Execute(Action<string>, bool, bool, IDbConnection, > > TextWriter) methods do not. I find this a little misleading and would > > comment the latter two. > > > Second, when debugging through, I noticed that in Driver/ > > DriverBase.cs, the SetCommandTimeout method (starting on line 139) > > takes two parameters, but the second parameter (the timeout value) is > > never used inside the method. A member variable is used instead. So > > I propose to remove that second, unused parameter from the method > > signature. > > > The cleanups (1) some comments, and (2) removing an unused method > > parameter, are pretty clearly not risky changes. > > > I figure fixing anything I notice is useful, even if small or not > > related to an actual bug. > > > I hope this clarifies and is helpful. > > > Chris > > > On Mar 4, 10:21 pm, Fabio Maulo <[email protected]> wrote: > > > Which is exactly the target of what you want do ? > > > Which are improvements or problems you are trying to solve ? > > > > I didn't see any problem or particular issue there. > > > > 2010/3/5 Chris Collier <[email protected]> > > > > > OK, will do. I'll proceed that way. What's the best form to provide > > > > the test cases in? Patch to appropriate test fixture class in > > > > NHibernate.Test project? > > > > > Thanks, > > > > Chris > > > > > On Mar 4, 6:07 pm, "Richard Brown \(gmail\)" <[email protected]> > > > > wrote: > > > > > Hi Chris, > > > > > > Obviously nobody can guarantee that a patch will be accepted. Your > > best > > > > bet > > > > > is to do address each (ideally small) issue one at a time, and > > provide > > > > test > > > > > cases with all patches. > > > > > > If there isn't actually a bug, then I suspect more detail would be > > > > helpful. > > > > > Probably as an improvement in JIRA. > > > > > > Cheers, > > > > > Richard > > > > > > -------------------------------------------------- > > > > > From: "Chris Collier" <[email protected]> > > > > > Sent: Thursday, March 04, 2010 9:43 PM > > > > > To: "nhibernate-development" < > > [email protected]> > > > > > Subject: [nhibernate-development] hbm2ddl patch; proposal for more > > > > > comprehensive work > > > > > > > I recently submitted this patch: > >http://216.121.112.228/browse/NH-2126, > > > > > > which I needed for work on the product my team is building using > > NH. > > > > > > In getting into the code, it looks like the hbm2ddl code could use > > a > > > > > > more complete fix to the issue I addressed: IDbCommand objects not > > > > > > being created using the same mechanisms the rest of NH uses (when > > > > > > appropriate). > > > > > > > In my opinion, the way the Execute(...) methods are chained could > > use > > > > > > some cleaning up as well. > > > > > > > I'd like to invest some time in giving some TLC to the hbm2ddl code > > -- > > > > > > does anyone support this effort, i.e., if I do it, assuming it's > > > > > > quality code, what are the chances that these hbm2ddl cleanups > > would > > > > > > be considered a worthwhile effort for 3.0? I can give more detail > > > > > > about my planned changes if wanted. Please advise. > > > > > > > Thanks > > > > > > Chris > > > > -- > > > Fabio Maulo > > -- > Fabio Maulo
