-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Subdirectories #1567
Subdirectories #1567
Conversation
78b44f9
to
ab4a560
Compare
@@ -6,7 +6,7 @@ public class KOSDeprecationException : KOSException | |||
{ | |||
protected static string TerseMessageFmt = "As of kOS {0}, {1} is obsolete and has been replaced with {2}"; | |||
|
|||
public KOSDeprecationException(string version, string oldUsage,string newUsage, string url) : | |||
public KOSDeprecationException(string version, string oldUsage,string newUsage) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two problems with this change:
(1) - It's got NOTHING to do with what you're trying to change in this PR.
(2) - If you had a problem with this being a parameter that wasn't used, the proper fix would have been to make it be used as it was intended (i.e. pass it down to the base KOSException class, and have KOSException populate helpURL from it), rather than remove it. It's meant to be a feature we can hook into in a later refactor, yes the infrastructure to actually take advantage of it is only half there, but removing it entirely because it's only half done feels like taking a step backward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I'll revert this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being harsh about it. I just really want that feature to work some day (the help URL). I know it's just a stub at the moment that doesn't do much yet.
Also, the consequences of this change, percolating through the rest of the code, were bloating the "files changed" count, and I was trying to narrow it down to which files I really needed to be looking at to see what changed for subdirectories.
0245236
to
c2e6221
Compare
Rebased on top of develop-1.1 (KSP 1.1) |
69f00e6
to
fd31c6b
Compare
@tomekpiotrowski is the "pending documentation" label still appropriate for this PR or are you done with that now? |
There's some documentation missing, primarily the Commands->File i/O page. It needs a few more hours of work. |
:return: :struct:`Path` | ||
|
||
Will return a new path with the extension of the last segment of this path replaced (or added if there's none). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for making the extension different from the rest of the name, in the sense that the extension can be changed with a suffix method but the rest of the name cannot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the way to change the rest of the path name be to do a chain of
path:parent:combine("newname.ks").
Or are you specifically looking for a way to generate a path with the same folder and extension but a different file name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Unix world (and therefore also Macs from OSX onward), the extension was never a separate special field. The filename IS just one string including the extension. (From the way the filesystem "thinks" in most UNIX systems, the humans who choose the filename strings seem to like using ones with exactly one period character in them a lot of the time for some mysterious reason the filesystem doesn't understand or care about.) The notion of an "extension" is a higher level logical abstraction consisting of that substring of the whole filename that comes after the lastmost period, if there is one. Things like GUI file managers, and system libraries, apply this abstraction on top. The filesystem itself has no such thing as "extensions".
For all I know the newer Windows filesystems may be the same way now too. The notion of a file "extension" came from DOS and FAT originally.
So this is why it looks very arbitrary and weird to me for there to be a rule that you can change the extension but not the rest of the name, via the PATH suffixes. The extension is nothing more than a subset of the filename. Either you should be able to change it via PATH, or you shouldn't be able to. Not this strange "it depends on which part of it you're trying to change".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point.
To be honest IIRC I took a couple of suffixes straight from C#'s Path class, this was one of them. I thought it could be useful to expose that functionality to the user. I see no problem with adding a similar CHANGENAME
suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the description in the RST file, I didn't understand how the combine
suffix works.
If it really is just "return a new path with a new filename in the same directory as this path", then it gives me what I'm looking for. I'm not looking for a suffix that ONLY changes the base substring of the name. I see that as just as arbitrary as one that only changes the extension substring of the name. I'm looking for how to change the whole name in one go, and treat it like it really is - as a single string - ignoring the "base dot extension" abstraction layer entirely and just changing the file's actual raw name, which includes the base, the period, and the extension in one string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with CHANGENAME
iff path:changename(newname)
does exactly the same thing as path:parent:combine(newname)
currently does, but not if it only works on the base name and not the extension. (The thing @hvacengi was guessing I might have meant up above is definitely not what I wanted.) We need to be fundamentally consistent in our terminology. When we say "file name" elsewhere, we do include the extension (i.e. path:name
returns "prog.ks" rather than just "prog") :changename
should be the same and the "name" it is changing should be the same "name" that :name
is returning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my intention to make it behave like path:parent:combine(newname)
.
I assumed extension to be the part of the file name after last dot (if any). So file name includes extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love it if we could get to a point where we always refer to files with the extension. run test.
should run a file named "test" not "test.ks" (or "test.ksm", that inference is part of the problem).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the behavior that was introduced by one of my previous PRs is a good compromise between what you mention and being backwards compatible.
That is when you create a file you need to specify the full name, no extensions are added by kOS. However when you access a file kOS tries to add '.ks' and '.ksm' to the file name if it doesn't find the original one. I've kept the same behavior in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added PATH:CHANGENAME(name)
I should have some time to work on this tomorrow and finish the documentation. |
great! I'm using it a bit, trying to work from just the small bits of documentation I can see, and from looking at the code to see the function names (I gave up on trying to "black box" test it, because I can't do that without all the docs). |
Ok, I think I'm done with documentation. I will review and test everything some more and upload updates if necessary, but it should be ready. |
This reverts commit 9ee3289.
It appears to have not been used, but was causing exceptions in certain cases
d678593
to
05527a5
Compare
Fixes #1364, fixes #778, fixes #754, fixes #1136, fixes #969, fixes #1536
I've started to update my old subdirectories code. I should have time to work on it in the next few days and I hope to make solid progress on the whole subdirectories front.
Breaking changes:
boot
directory.delete filename from volume
,copy filename from/to volume
,rename (both forms)
have been deprecated in favor ofdeletepath(path)
,copypath(fromPath, toPath)
,movepath(fromPath, toPath)
andset volume:name to newname
.Here's an outline of things to do:
VolumeDirectory
typeGlobalPath
Path
typepath()
command to create paths (path
with no arguments creates aPath
for the current path)VolumeDirectory
as a userspace typevolume()
command, to get a volume by name or idVolume
suffixes should accept strings representingVolumePath
scopypath()
commandmovepath()
commandPath
boot
directory as bootfilesPath
documentationVolume
suffixespath()
volume()
scriptpath()
cd()
copypath()
,movepath()
,deletepath()
createdir()
create()
,exists()
andopen()
VolumeItem
documentationVolumeDirectory
documentation