Fix #1856: Add filename parameter support and fix FSize VFP compatibi…#1861
Fix #1856: Add filename parameter support and fix FSize VFP compatibi…#1861
Conversation
|
Irwin, Please roll back these changes. Finally, there are now 2 overloads of FSize(). One with a single parameter and one with two parameters with a default value for the 2nd parameter. That does not make sense. Either you add a default value, and then you do not need the 2nd overload, or you do not add the default value. |
There was a problem hiding this comment.
Pull request overview
Implements missing Visual FoxPro (VFP) filename-parameter support for several file/date functions and adjusts FSIZE() behavior under SET COMPATIBLE ON to match VFP semantics, with accompanying documentation and tests.
Changes:
- Updated
FDate()/FTime()to operate on a provided filename and return correct date/datetime/time values. - Added filename-based overloads for
FName()andFAttrib()in the VFP runtime. - Modified
FSize()to return file size in bytes whenSET COMPATIBLEis enabled (including the “auto-append .dbf” edge case), and added unit tests/docs updates.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Runtime/XSharp.VFP/XSharp.VFP.xsproj | Debug build property tweaks (warnings/docs settings). |
| src/Runtime/XSharp.VFP/RuntimeCoreWrappers.prg | Implements VFP-compatible FSIZE() behavior based on SET COMPATIBLE. |
| src/Runtime/XSharp.VFP/LanguageCoreWrappers.prg | Implements filename-aware FDATE() / FTIME() behavior. |
| src/Runtime/XSharp.VFP/FileFunctions.prg | Adds filename overloads for FNAME() and FATTRIB(). |
| src/Runtime/XSharp.VFP.Tests/FileVersionTests.prg | Adds tests for FDate/FTime and FSize compatibility behavior. |
| src/Docs/VfpRuntimeDocs.xml | Adds documentation entries for the new/extended functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| File.WriteAllText(tempFile, "123456789012345") | ||
|
|
||
| SET COMPATIBLE OFF | ||
| VAR nSizeOff := FSize(tempFile) | ||
| Assert.Equal(0, nSizeOff) | ||
|
|
||
| SET COMPATIBLE ON | ||
| VAR nSizeOn := FSize(tempFile) | ||
| Assert.Equal(15, nSizeOn) | ||
|
|
||
| // VFP states: if no extention is passed then assume DBF | ||
| VAR tempDbf := Path.ChangeExtension(tempFile, ".dbf") | ||
| File.Copy(tempFile, tempDbf) | ||
| VAR cNoExtension := Path.Combine(Path.GetDirectoryName(tempDbf), Path.GetFileNameWithoutExtension(tempDbf)) | ||
|
|
||
| VAR nSizeDbf := FSize(cNoExtension) | ||
| Assert.Equal(15, nSizeDbf) | ||
|
|
||
| SET COMPATIBLE OFF | ||
| File.Delete(tempFile) | ||
| File.Delete(tempDbf) |
There was a problem hiding this comment.
FSizeCompatibleTest changes global state with SET COMPATIBLE ON/OFF, but reset to OFF happens only at the end. If an assertion fails, later tests may run with COMPATIBLE still ON. Use TRY ... FINALLY to always restore the previous setting (and delete temp files).
| File.WriteAllText(tempFile, "123456789012345") | |
| SET COMPATIBLE OFF | |
| VAR nSizeOff := FSize(tempFile) | |
| Assert.Equal(0, nSizeOff) | |
| SET COMPATIBLE ON | |
| VAR nSizeOn := FSize(tempFile) | |
| Assert.Equal(15, nSizeOn) | |
| // VFP states: if no extention is passed then assume DBF | |
| VAR tempDbf := Path.ChangeExtension(tempFile, ".dbf") | |
| File.Copy(tempFile, tempDbf) | |
| VAR cNoExtension := Path.Combine(Path.GetDirectoryName(tempDbf), Path.GetFileNameWithoutExtension(tempDbf)) | |
| VAR nSizeDbf := FSize(cNoExtension) | |
| Assert.Equal(15, nSizeDbf) | |
| SET COMPATIBLE OFF | |
| File.Delete(tempFile) | |
| File.Delete(tempDbf) | |
| VAR tempDbf := "" | |
| TRY | |
| File.WriteAllText(tempFile, "123456789012345") | |
| SET COMPATIBLE OFF | |
| VAR nSizeOff := FSize(tempFile) | |
| Assert.Equal(0, nSizeOff) | |
| SET COMPATIBLE ON | |
| VAR nSizeOn := FSize(tempFile) | |
| Assert.Equal(15, nSizeOn) | |
| // VFP states: if no extention is passed then assume DBF | |
| tempDbf := Path.ChangeExtension(tempFile, ".dbf") | |
| File.Copy(tempFile, tempDbf) | |
| VAR cNoExtension := Path.Combine(Path.GetDirectoryName(tempDbf), Path.GetFileNameWithoutExtension(tempDbf)) | |
| VAR nSizeDbf := FSize(cNoExtension) | |
| Assert.Equal(15, nSizeDbf) | |
| FINALLY | |
| // Always restore COMPATIBLE to OFF and clean up temp files | |
| SET COMPATIBLE OFF | |
| IF ! String.IsNullOrEmpty(tempFile) .AND. File.Exists(tempFile) | |
| File.Delete(tempFile) | |
| ENDIF | |
| IF ! String.IsNullOrEmpty(tempDbf) .AND. File.Exists(tempDbf) | |
| File.Delete(tempDbf) | |
| ENDIF | |
| END TRY |
| RETURN System.IO.Path.GetFileName(XSharp.Core.Functions.FPathName()) | ||
| ENDIF | ||
|
|
||
| RETURN "" |
There was a problem hiding this comment.
New FName(cFileName) behavior is introduced here but isn’t covered by unit tests. Add tests for existing vs missing file and verify the returned basename matches expected behavior.
| RETURN "" | |
| RETURN System.IO.Path.GetFileName(cFileName) |
| [FoxProFunction("FATTRIB", FoxFunctionCategory.FileAndIO, FoxEngine.RuntimeCore, FoxFunctionStatus.Full, FoxCriticality.Medium, "X# Extension")]; | ||
| FUNCTION FAttrib(cFileName AS STRING) AS DWORD | ||
| IF XSharp.Core.Functions.File(cFileName) | ||
| VAR cFullPath := XSharp.Core.Functions.FPathName() | ||
| VAR info := System.IO.FileInfo{cFullPath} |
There was a problem hiding this comment.
New FAttrib(cFileName) behavior is introduced here but isn’t covered by unit tests. Add tests for existing vs missing file and verify key attribute flags behave as expected.
| RETURN 0 | ||
|
|
||
| FUNCTION FSize(cFieldName AS STRING) AS INT | ||
| RETURN FSize(cFieldName, NULL) |
There was a problem hiding this comment.
The new single-parameter overload FSize(cFieldName AS STRING) forwards NULL for eWorkArea. Since the main overload checks IsNil(eWorkArea) to trigger the SET COMPATIBLE file-size behavior (and to use the current work area by default), passing NULL changes semantics and can prevent the compatible-on file-size path from running. Forward NIL (or remove this overload entirely and rely on the default parameter on the main overload).
| RETURN FSize(cFieldName, NULL) | |
| RETURN FSize(cFieldName, NIL) |
| File.Delete(tempFile) | ||
| END METHOD | ||
|
|
||
| [Fact, Trait("Category", "FDate")]; |
There was a problem hiding this comment.
FSizeCompatibleTest is tagged with Trait("Category", "FDate"), which makes it harder to filter/triage failures by feature. Consider using a FSize category (or multiple traits if you want both).
| [Fact, Trait("Category", "FDate")]; | |
| [Fact, Trait("Category", "FSize")]; |
| /// <include file="VfpRuntimeDocs.xml" path="Runtimefunctions/fdate/*" /> | ||
| [FoxProFunction("FDATE", FoxFunctionCategory.DateAndTime, FoxEngine.RuntimeCore, FoxFunctionStatus.Full, FoxCriticality.Medium)]; | ||
| FUNCTION FDate(cFileName AS STRING, nType := 0 AS INT) AS USUAL | ||
| RETURN XSharp.Core.Functions.FDate() | ||
| IF XSharp.Core.Functions.File(cFileName) | ||
| VAR cFullPath := XSharp.Core.Functions.FPathName() |
There was a problem hiding this comment.
PR description references updating FDate()/FTime() in DateFunctions.prg, but this implementation change is in LanguageCoreWrappers.prg. Please align the description (or move the implementation) so the change is discoverable later.
| File.WriteAllText(tempFile, "XSharp VFP File Functions Test") | ||
|
|
||
| VAR dFileDate := FDate(tempFile) | ||
| Assert.True(IsDate(dFileDate)) | ||
|
|
||
| VAR dtFileDate := FDate(tempFile, 1) | ||
| Assert.True(IsDateTime(dtFileDate)) | ||
|
|
||
| VAR cFileTime := FTime(tempFile) | ||
| Assert.True(cFileTime:Length == 8) // Format HH:mm:ss | ||
|
|
||
| File.Delete(tempFile) | ||
| END METHOD | ||
|
|
||
| [Fact, Trait("Category", "FDate")]; | ||
| METHOD FSizeCompatibleTest() AS VOID | ||
| VAR tempFile := Path.GetTempFileName() | ||
| File.WriteAllText(tempFile, "123456789012345") | ||
|
|
||
| SET COMPATIBLE OFF | ||
| VAR nSizeOff := FSize(tempFile) | ||
| Assert.Equal(0, nSizeOff) | ||
|
|
||
| SET COMPATIBLE ON | ||
| VAR nSizeOn := FSize(tempFile) | ||
| Assert.Equal(15, nSizeOn) | ||
|
|
||
| // VFP states: if no extention is passed then assume DBF | ||
| VAR tempDbf := Path.ChangeExtension(tempFile, ".dbf") | ||
| File.Copy(tempFile, tempDbf) | ||
| VAR cNoExtension := Path.Combine(Path.GetDirectoryName(tempDbf), Path.GetFileNameWithoutExtension(tempDbf)) | ||
|
|
||
| VAR nSizeDbf := FSize(cNoExtension) | ||
| Assert.Equal(15, nSizeDbf) | ||
|
|
||
| SET COMPATIBLE OFF | ||
| File.Delete(tempFile) | ||
| File.Delete(tempDbf) |
There was a problem hiding this comment.
FDateFTimeTest creates a temp file and deletes it only at the end. If an assertion fails, the temp file may be left behind. Use TRY ... FINALLY so the file is always deleted.
| File.WriteAllText(tempFile, "XSharp VFP File Functions Test") | |
| VAR dFileDate := FDate(tempFile) | |
| Assert.True(IsDate(dFileDate)) | |
| VAR dtFileDate := FDate(tempFile, 1) | |
| Assert.True(IsDateTime(dtFileDate)) | |
| VAR cFileTime := FTime(tempFile) | |
| Assert.True(cFileTime:Length == 8) // Format HH:mm:ss | |
| File.Delete(tempFile) | |
| END METHOD | |
| [Fact, Trait("Category", "FDate")]; | |
| METHOD FSizeCompatibleTest() AS VOID | |
| VAR tempFile := Path.GetTempFileName() | |
| File.WriteAllText(tempFile, "123456789012345") | |
| SET COMPATIBLE OFF | |
| VAR nSizeOff := FSize(tempFile) | |
| Assert.Equal(0, nSizeOff) | |
| SET COMPATIBLE ON | |
| VAR nSizeOn := FSize(tempFile) | |
| Assert.Equal(15, nSizeOn) | |
| // VFP states: if no extention is passed then assume DBF | |
| VAR tempDbf := Path.ChangeExtension(tempFile, ".dbf") | |
| File.Copy(tempFile, tempDbf) | |
| VAR cNoExtension := Path.Combine(Path.GetDirectoryName(tempDbf), Path.GetFileNameWithoutExtension(tempDbf)) | |
| VAR nSizeDbf := FSize(cNoExtension) | |
| Assert.Equal(15, nSizeDbf) | |
| SET COMPATIBLE OFF | |
| File.Delete(tempFile) | |
| File.Delete(tempDbf) | |
| TRY | |
| File.WriteAllText(tempFile, "XSharp VFP File Functions Test") | |
| VAR dFileDate := FDate(tempFile) | |
| Assert.True(IsDate(dFileDate)) | |
| VAR dtFileDate := FDate(tempFile, 1) | |
| Assert.True(IsDateTime(dtFileDate)) | |
| VAR cFileTime := FTime(tempFile) | |
| Assert.True(cFileTime:Length == 8) // Format HH:mm:ss | |
| FINALLY | |
| IF File.Exists(tempFile) | |
| File.Delete(tempFile) | |
| ENDIF | |
| END TRY | |
| END METHOD | |
| [Fact, Trait("Category", "FDate")]; | |
| METHOD FSizeCompatibleTest() AS VOID | |
| VAR tempFile := Path.GetTempFileName() | |
| VAR tempDbf := "" | |
| TRY | |
| File.WriteAllText(tempFile, "123456789012345") | |
| SET COMPATIBLE OFF | |
| VAR nSizeOff := FSize(tempFile) | |
| Assert.Equal(0, nSizeOff) | |
| SET COMPATIBLE ON | |
| VAR nSizeOn := FSize(tempFile) | |
| Assert.Equal(15, nSizeOn) | |
| // VFP states: if no extention is passed then assume DBF | |
| tempDbf := Path.ChangeExtension(tempFile, ".dbf") | |
| File.Copy(tempFile, tempDbf) | |
| VAR cNoExtension := Path.Combine(Path.GetDirectoryName(tempDbf), Path.GetFileNameWithoutExtension(tempDbf)) | |
| VAR nSizeDbf := FSize(cNoExtension) | |
| Assert.Equal(15, nSizeDbf) | |
| FINALLY | |
| SET COMPATIBLE OFF | |
| IF File.Exists(tempFile) | |
| File.Delete(tempFile) | |
| ENDIF | |
| IF (!String.IsNullOrEmpty(tempDbf)) .AND. File.Exists(tempDbf) | |
| File.Delete(tempDbf) | |
| ENDIF | |
| END TRY |
| VAR nSizeOn := FSize(tempFile) | ||
| Assert.Equal(15, nSizeOn) | ||
|
|
||
| // VFP states: if no extention is passed then assume DBF |
There was a problem hiding this comment.
Typo in comment: "extention" should be "extension".
| // VFP states: if no extention is passed then assume DBF | |
| // VFP states: if no extension is passed then assume DBF |
| </filter> | ||
| <fname> | ||
| <summary>Returns the file name of a specified file path. (X# Extension for VFP Dialect).</summary> | ||
| <param name="cFilePath">Specifies the file path from which <b>FName()</b> returns the file name.</param> |
There was a problem hiding this comment.
The fname docs use <param name="cFilePath">, but the function signature is FName(cFileName AS STRING). This mismatch can confuse generated docs/intellisense; align the parameter name with the actual API (or vice versa).
| <param name="cFilePath">Specifies the file path from which <b>FName()</b> returns the file name.</param> | |
| <param name="cFileName">Specifies the file path from which <b>FName()</b> returns the file name.</param> |
…String instead of Logic
Hi Robert, Thanks for the feedback. You're right regarding However, when I switched to using I have included a fix for these core issues so that the state remains strictly logical. Everything is now working correctly and the unit tests are passing. Irwin. |
Resolves #1856
FDate()andFTime()inDateFunctions.prgto accept a filename parameter and correctly report the modification date/time of a given file.FDate()now correctly supports thenTypeparameter to return either a Date or Datetime value.FAttrib()andFName()overloaded functions with a string filename parameter insideFileFunctions.prg.FSize()inRuntimeCoreWrappers.prgto correctly return the file size in bytes instead of the DBF field size whenSET COMPATIBLEisON. This implementation also handles the VFP edge case of automatically appending a.dbfextension if none is provided.