Page MenuHome

Proposal for new names in `BLI_path_util`
Confirmed, NormalPublicTO DO

Description

This is my proposal for new names in BLI_path_util.h and path_util.c. The original names are listed first, the suggested names second.

This is not finished (I'll remove this sentence once it is), it's work in progress. However, feel free to comment on what is described here already.

// IMO these should not be in BLI_path_util, as they are only slightly related to paths.
void BLI_setenv(const char *env, const char *val);
void BLI_setenv_if_new(const char *env, const char *val);
const char *BLI_getenv(const char *env);

// Performs multiple operations at once:
// - join 'dir' and 'file' to form a full path, and
// - resolve paths starting with '//' relative to the 'relabase' filename.
// I renamed it but also moved the return variable 'string' in the middle of the parameters to
// 'r_path' at the end.
void BLI_make_file_string(const char *relabase, char *string, const char *dir, const char *file);
void BLI_path_join_and_resolve(const char *rel_base_file,
                               const char *dir,
                               const char *file,
                               char *r_path);

// Ensures the directory exists, given a hypothetical file path in that directory.
bool BLI_make_existing_file(const char *name);
bool BLI_path_ensure_directory_exists(const char *file_path);

// Splits a full path into a directory and basename component.
// The basename can be a file or a directory, depending on the path that was given to it.
void BLI_split_dirfile(
    const char *string, char *dir, char *file, const size_t dirlen, const size_t filelen);
void BLI_path_split(const char *path,
                    char *r_dirname,
                    char *r_basename,
                    const size_t dirlen,
                    const size_t baselen);

// Extracts the dirname from a full path.
void BLI_split_dir_part(const char *string, char *dir, const size_t dirlen);
void BLI_path_dirname(const char *path, char *r_dirname, const size_t dirlen);

// Extracts the filename from a full path.
void BLI_split_file_part(const char *string, char *file, const size_t filelen);
void BLI_path_basename(const char *path, char *r_filename, const size_t filelen);

// Fine as it is.
const char *BLI_path_extension(const char *filepath);

Event Timeline

Sybren A. Stüvel (sybren) changed the task status from Needs Triage to Confirmed.Mar 6 2020, 5:49 PM
Sybren A. Stüvel (sybren) created this task.
Sybren A. Stüvel (sybren) changed the subtype of this task from "Report" to "To Do".

void BLI_path_basename(const char *path, char *r_filename, const size_t filelen);

parameter name is r_filename, so the size field ought to be called const size_t filenamelen

same applies to some of the other functions.

Note, some terminology we could settle on before going into details, following Python conventions and some conventions already in Blender's code.

  • Term for the directory component of a path dirname.
  • Term for the file without any leading path basename.
  • Term for a directory - for functions which only operate on directories dir.
  • Term for a directory file - for functions which only operate on names filename.
  • Term of a complete path filepath.
  • BLI_setenv: We could add BLI_os_ for OS wrappers (BLI_setenv -> BLI_os_setenv), some functions from storage.c could eventually be moved there too BLI_wstat for eg.
  • BLI_path_join_and_resolve +1

    In this case resolve is ambiguous, although including all the functionality of this call in it's name could get us something like BLI_path_join_and_ensure_absolute_with_native_slashes.

    Although this function is fairly spesific can could be replaced by BLI_join_dirfile in many instances (edit, done, it's only used in a few places now).
  • BLI_path_ensure_directory_exists:

    The function is awkward since it's extracting the parent dir, then ensuring that exists. The proposed naming could be confused with a function that simply ensures the directory name passed in exists.

    What about BLI_path_ensure_dirname_exists? Making it clear the directory component of the input argument is being used.
  • BLI_path_split +1

    Although this isn't the reverse of BLI_path_join, it' matches Python's os.path.split, and I never found it a problem there.
  • BLI_path_dirname / BLI_path_basename: ... there is a complication.

    We already have BLI_path_basename which returns the path instead of copying into a new buffer.

    We could keep both these functions as being varients of BLI_path_split, e.g:

    BLI_path_split_dirname, BLI_path_split_basename.

Further Proposals for Renaming

# Naming: from -> to.
replace_all = (
    # Use `BLI_path_` prefix.
    ("BLI_parent_dir",                "BLI_path_parent_dir"),
    ("BLI_parent_dir_until_exists",   "BLI_path_parent_dir_until_exists"),
    ("BLI_ensure_filename",           "BLI_path_filename_ensure"),
    ("BLI_first_slash",               "BLI_path_slash_find"),
    ("BLI_last_slash",                "BLI_path_slash_rfind"),
    ("BLI_add_slash",                 "BLI_path_slash_ensure"),
    ("BLI_del_slash",                 "BLI_path_slash_rstrip"),
    ("BLI_path_native_slash",         "BLI_path_slash_native"),

    # Rename 'cleanup' to 'normalize',
    # Similar to Python's `os.path.normpath`.
    ("BLI_cleanup_path", "BLI_path_normalize"),
    ("BLI_cleanup_dir",  "BLI_path_normalize_dir"),
    ("BLI_cleanup_unc",  "BLI_path_normalize_unc"),
    ("BLI_cleanup_unc16","BLI_path_normalize_unc16"),

    # Names are just suggestions, no strong opinion here.
    ("BLI_stringenc","BLI_path_name_and_number_encode"),
    ("BLI_stringdec","BLI_path_name_and_number_decode"),
    # Possible alternative:
    ("BLI_stringenc","BLI_path_sequence_encode"),
    ("BLI_stringdec","BLI_path_sequence_decode"),

)

Note, some terminology we could settle on before going into details, following Python conventions and some conventions already in Blender's code.

👍

  • Term for the directory component of a path dirname.
  • Term for the file without any leading path basename.
  • Term of a complete path filepath.

👍

  • Term for a directory - for functions which only operate on directories dir.

I usually try to distinguish between "the directory name" and "the directory". This is mostly relevant for files, though, where I would consider something like a FILE * as "file", and char * as "filename".

  • Term for a directory - for functions which only operate on names filename.

I'm guessing this is a copy-paste error and it's actually a term for a filename, not a directory.

  • BLI_setenv: We could add BLI_os_ for OS wrappers (BLI_setenv -> BLI_os_setenv), some functions from storage.c could eventually be moved there too BLI_wstat for eg.

👍

  • BLI_path_join_and_resolve +1

    In this case resolve is ambiguous, although including all the functionality of this call in it's name could get us something like BLI_path_join_and_ensure_absolute_with_native_slashes.

    Although this function is fairly spesific can could be replaced by BLI_join_dirfile in many instances (edit, done, it's only used in a few places now).

I feel the same way, to me it's an indication that the function is doing too much at once. I'd be happy with two functions, one for resolving //-relative paths, and one for joining dirname and filename.

  • BLI_path_ensure_directory_exists:

    The function is awkward since it's extracting the parent dir, then ensuring that exists. The proposed naming could be confused with a function that simply ensures the directory name passed in exists.

Although it makes total sense to me from a naming standpoint, this function is used a lot when trying to write a file. It's quite convenient to have a single function "make sure I can write this file".

What about `BLI_path_ensure_dirname_exists`? Making it clear the directory component of the input argument is being used.

This is why I try to make a distinction between "directory" and "directory name": a directory name always exists. BLI_path_ensure_dir_exists is fine for me, or maybe BLI_path_mkparent? At least it reads a bit like mkdir and it's clear it's about the parent.

  • BLI_path_dirname / BLI_path_basename: ... there is a complication.

    We already have BLI_path_basename which returns the path instead of copying into a new buffer.

    We could keep both these functions as being varients of BLI_path_split, e.g:

    BLI_path_split_dirname, BLI_path_split_basename.

This is a complication indeed. If there are two functions that do the same, and only differ in their memory management (overwrite existing mem vs. allocate new mem) I wouldn't mind if they were named the same except for some small difference that indicates which one of the two it is. Maybe BLI_path_basename and BLI_path_basename_alloc? I know this is troublesome for patches, as the new name of the non-allocating function would be the same as the old name of the allocating one, potentially creating nasty bugs. However, I'd rather think of what naming scheme we actually want to use, and then think of how to deal with the renaming.

  1. Further Proposals for Renaming

All 👍 for me.

  1. Names are just suggestions, no strong opinion here. ("BLI_stringenc","BLI_path_name_and_number_encode"), ("BLI_stringdec","BLI_path_name_and_number_decode"),
  2. Possible alternative: ("BLI_stringenc","BLI_path_sequence_encode"), ("BLI_stringdec","BLI_path_sequence_decode"),

I think the alternatives here are clearer. To me they are, anyway.

This task is a bit big, I'd like to split off partially related things where possible:

  • Term for a directory - for functions which only operate on directories dir.

I usually try to distinguish between "the directory name" and "the directory". This is mostly relevant for files, though, where I would consider something like a FILE * as "file", and char * as "filename".

It makes sense in general, however we have BLI_path_ as a prefix, to show that it's operating on paths, not the file/directory arguments.

  • Term for a directory - for functions which only operate on names filename.

I'm guessing this is a copy-paste error and it's actually a term for a filename, not a directory.

Yes, corrected.

  • BLI_setenv: We could add BLI_os_ for OS wrappers (BLI_setenv -> BLI_os_setenv), some functions from storage.c could eventually be moved there too BLI_wstat for eg.

👍

Submitted T75516: Day of Clean Code: Split wrappers for C API functions into own files.

  • BLI_path_join_and_resolve +1

    In this case resolve is ambiguous, although including all the functionality of this call in it's name could get us something like BLI_path_join_and_ensure_absolute_with_native_slashes.

    Although this function is fairly spesific can could be replaced by BLI_join_dirfile in many instances (edit, done, it's only used in a few places now).

I feel the same way, to me it's an indication that the function is doing too much at once. I'd be happy with two functions, one for resolving //-relative paths, and one for joining dirname and filename.

In this case I think we could leave the name as-is and remove it's use.
Or at least not make it part of this renaming task.

  • BLI_path_ensure_directory_exists:

    The function is awkward since it's extracting the parent dir, then ensuring that exists. The proposed naming could be confused with a function that simply ensures the directory name passed in exists.

Although it makes total sense to me from a naming standpoint, this function is used a lot when trying to write a file. It's quite convenient to have a single function "make sure I can write this file".

What about `BLI_path_ensure_dirname_exists`? Making it clear the directory component of the input argument is being used.

This is why I try to make a distinction between "directory" and "directory name": a directory name always exists. BLI_path_ensure_dir_exists is fine for me, or maybe BLI_path_mkparent? At least it reads a bit like mkdir and it's clear it's about the parent.

BLI_path_mkparent seems fine, slight preference for BLI_path_parent_ensure since it's more in keeping with other names.

  • BLI_path_dirname / BLI_path_basename: ... there is a complication.

    We already have BLI_path_basename which returns the path instead of copying into a new buffer.

    We could keep both these functions as being varients of BLI_path_split, e.g:

    BLI_path_split_dirname, BLI_path_split_basename.

This is a complication indeed. If there are two functions that do the same, and only differ in their memory management (overwrite existing mem vs. allocate new mem) I wouldn't mind if they were named the same except for some small difference that indicates which one of the two it is. Maybe BLI_path_basename and BLI_path_basename_alloc? I know this is troublesome for patches, as the new name of the non-allocating function would be the same as the old name of the allocating one, potentially creating nasty bugs. However, I'd rather think of what naming scheme we actually want to use, and then think of how to deal with the renaming.

Neither allocate, one just copies into an existing buffer, while we could do something like what you suggest, call it

BLI_path_basename_copy, this wouldn't be in keeping with many other functions in this API that copy their output.

So I'd still prefer BLI_path_split_* functions as suggested.

  1. Further Proposals for Renaming

All 👍 for me.

  1. Names are just suggestions, no strong opinion here. ("BLI_stringenc","BLI_path_name_and_number_encode"), ("BLI_stringdec","BLI_path_name_and_number_decode"),
  2. Possible alternative: ("BLI_stringenc","BLI_path_sequence_encode"), ("BLI_stringdec","BLI_path_sequence_decode"),

I think the alternatives here are clearer. To me they are, anyway.

Done (second alternatives).