Well, there are two issues: * We don't always call IFN.segmentFileName -- often we simply concatenate strings directly throughout the sources.
* Should the extensions include '.' or not? I'd really like to fix the first issue. If we do that, then there's only 1 place that performs string concatenation when building file names, and then we are free to alter whether extensions include '.' or not, but... On the 2nd issue, I think it's sort of trivial -- we could have gone either way -- kinda like how Intel and Sun could've gone either way when they picked byte order ;) -- but since we've been doing no dots for so long, I'm not sure we should suddenly change that "convention" now. Performance cost should be miniscule right? I mean creating file names shouldn't be happening in any hot spots... Mike On Tue, Feb 23, 2010 at 6:22 AM, Shai Erera <[email protected]> wrote: > But segmentFileName performs the concatenation internally: > static String segmentFileName(String segmentName, String ext) { > return segmentName + "." + ext; > } > So that would not avoid anything right? > > And still, if someone needs to know whether a certain file is a core Lucene > file or his own added file, he'll need to add the ".". I guess I don't > understand why not to define the constants with the ".'? Although I didn't > do a through scan through the code, I didn't find in places I checked that a > reference to *just* the extension name is needed. > > And thanks for correcting me on the package-private and back-compat thing. > In my mind it was already public :). > > Shai > > On Tue, Feb 23, 2010 at 1:16 PM, Michael McCandless > <[email protected]> wrote: >> >> This class makes me somewhat nervous, with the changes coming in flex, >> because the extensions are no longer static but rather a function of >> the particular codec you're using in the index. I've changed some of >> the constants accordingly (on flex). >> >> Still, I think it's OK to make it public (flex has in fact done so, >> since codecs, in different packages, need to reference the constants), >> but mark it as @lucene.internal. >> >> There is no back compat issue here -- it's package private currently. >> Even once it's public with @lucene.internal, there's still no back >> compat since it's internal. >> >> I would love to never do our own concatenation in Lucene's sources, >> and instead consistently use IndexFileNames.segmentFileName method >> whenever we need to build up a file. Then it's OK that the extensions >> do not include the '.'? >> >> Mike >> >> On Tue, Feb 23, 2010 at 1:42 AM, Shai Erera <[email protected]> wrote: >> > Hi, >> > >> > I've noticed that IFN is package private, including its constants and >> > methods. Any reason why not make it public? I need to create my own >> > Directory, and would like to query whether a file is 'my file' or >> > Lucene's >> > file. Getting access to the extensions can help. Currently I have to put >> > my >> > code in o.a.l package, which I prefer not to. >> > >> > Also, I think the constants could be wrong in some situations. They only >> > include the extension itself, w/o the "." part. That leads to many >> > places in >> > the code where I see this <code>fname + "." + IFN.extension</code>. Two >> > problems: >> > (1) Doing "." + ext is just a waste - it's really not needed. >> > (2) If someone only checks for the extension, without adding the "." >> > part, >> > he can accidentally think that fnamedel is a deletion file .. >> > >> > I'd like to patch this, but want to validate with the community before I >> > create an issue. Making it public is trivial. Changing the extension >> > name is >> > more problematic for back-compat, but I think currently it's just wrong >> > ... >> > Unless we come up w/ new names for the extensions, which is annoying. >> > Can we >> > sacrifice back-compat for this thing? >> > >> > Thanks, >> > Shai >> > >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> For additional commands, e-mail: [email protected] >> > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
