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
