Skip to content
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

ChangeDirectoryCurrent #364

Closed
stevelinton opened this issue Nov 19, 2015 · 7 comments · Fixed by #5585
Closed

ChangeDirectoryCurrent #364

stevelinton opened this issue Nov 19, 2015 · 7 comments · Fixed by #5585
Labels
good first issue Issues that can be understood and addressed by newcomers to GAP development kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel
Milestone

Comments

@stevelinton
Copy link
Contributor

While I appreciate the general caution at adding features in the "core" system or moving them from packages, I think ChangeCurrentDirectory from IO might be an exceptional case.

Lots of people look for this functionality, and it's a trivial amount of code, while loading IO requires compiling a C module and may be problematic on systems that are far from POSIX.

One could argue for making IO a required package, but since it contains C code this would add a step to the minimal installation process and is extra tricky on Windows. It also requires loading a lot of code for (eg) pickling which most users will never need directly.

@fingolfin
Copy link
Member

I have no qualms about adding such a function, although of course the concept of a "current directory" is one that is fairly specific to Unix and Windows/DOS. But since most (all) desktop systems these days, there is probably no harm in ading that.

I"d still like to see proper path abstraction in GAP, like e.g. Python's OS path, but I don't think one should withold a useful feature just because one day one might perhaps be able to do it "better".

@stevelinton
Copy link
Contributor Author

We already have DirectoryCurrent() so we're not adding exposure to any more concepts.

@olexandr-konovalov
Copy link
Member

yes!!! I've missed it yesterday having teaching GAP to an audience with users of Linux, OS X and Windows. This would make things easier. Thanks for reminding, I forgot that this exists in IO. Just tested, all works on Windows. Note that the correct name is ChangeDirectoryCurrent.

@olexandr-konovalov olexandr-konovalov changed the title ChangeCurrentDirectory ChangeDirectoryCurrent Nov 19, 2015
@stevelinton
Copy link
Contributor Author

If we want to do this, some care is needed to do it cleanly. The first step is to release a version of IO that only installs this function if it is not already bound.

Once that is included in the distribution, we can update GAP to define this function.

Are there other functions from IO where there is a strong case to migrate them at the same time without changing their names?

@ChrisJefferson
Copy link
Contributor

I have added the relevant pull request to IO. This seems like the only future we could move from IO without a name change, as almost everything else starts IO_

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Mar 17, 2017
@fingolfin fingolfin added good first issue Issues that can be understood and addressed by newcomers to GAP development topic: kernel labels Nov 29, 2018
@fingolfin fingolfin added this to the GAP 4.13.0 milestone Dec 23, 2022
@fingolfin
Copy link
Member

To anyone interested in working on this, some notes:

  • the IO package already defines a function ChangeDirectoryCurrent, but it already does what was suggested above (i.e.: if IO detects that such a function already exists). That means you don't have to worry about clashing with this; only about staying compatible with it (you could even copy its implementation), and you can also borrow from its
  • you can possibly copy code from it
  • it probably would make sense to also look into improving DirectoryCurrent: it currently returns an object wrapping the path "." which works, but is probably not what most people except, which will want something more like what IO_getcwd produces -- namely an actual path to the current directory, which will also work as an argument to ChangeDirectoryCurrent when used in a different directory (so that one can "switch back")

@fingolfin
Copy link
Member

Implementation notes:

  • most of this will be C code, I think a natural place for it would be src/streams.c (which might be surprising but it already contains FuncCreateDir and FuncRemoveDir ... perhaps all of those should be moved into a new file, but I would advise against doing that in a PR implementing this feature request!)
  • add a new FuncChangeDir or so which takes a string and essentially wraps the chdir POSIX call. You can take inspiration from this IO function:
static Obj FuncIO_chdir(Obj self, Obj pathname)
{
    int res;
    if (!IS_STRING(pathname) || !IS_STRING_REP(pathname)) {
        SyClearErrorNo();
        return Fail;
    }

    res = chdir(CSTR_STRING(pathname));
    if (res < 0) {
        SySetErrorNo();
        return Fail;
    }
    return True;
}
  • this is low-level; a GAP function ChangeDirectoryCurrent could be implemented in lib/files.gd next to the implementation for DirectoryCurrent, which calls the new low-level function
  • this is also where a GAPDoc comment with the documentation would be added...
  • also edit doc/ref/files.xml to add <#Include Label="ChangeDirectoryCurrent">

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that can be understood and addressed by newcomers to GAP development kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel
Projects
None yet
4 participants