Allow hyphens in fpm project names in "fpm new" #337

milancurcic · 2021-01-12T22:20:53Z

fpm new currently requires a package name to be a valid Fortran name.

$ fpm new test-new
<ERROR>the new directory basename must be an allowed                    
       Fortran name. It must be composed of 1 to 63 ASCII               
       characters and start with a letter and be composed               
       entirely of alphanumeric characters [a-zA-Z0-9]                  
       and underscores.                                                 
STOP 4

To have a hyphen in a package name is rather common though. For example, we have stdlib-cmake-example, there's toml-f and others, and I enjoy using hyphens as well.

This PR removes the restriction to use hyphens in new fpm package names in fpm new. The function that I used to do that (to_fortran_name()) can be easily expanded to other special characters if a need arises.

If you try this PR, you should get:

$ ./fpm new test-new
 + mkdir -p test-new
 + cd test-new
 + mkdir -p test-new/src
 + mkdir -p test-new/test
 + mkdir -p test-new/app
 + git init test-new
Initialized empty Git repository in /your/path/to/fpm/build/gfortran_debug/app/test-new/.git/

$ cd test-new
$ head fpm.toml 
name = "test-new"
version = "0.1.0"
license = "license"
author = "Jane Doe"
maintainer = "jane.doe@example.com"
copyright = "2021 Jane Doe"


[library]
source-dir="src"

$ cat src/test-new.f90 
module test_new
  implicit none
  private

  public :: say_hello
contains
  subroutine say_hello
    print *, "Hello, test-new!"
  end subroutine say_hello
end module test_new

I also updated the error message that you get if you pass some other special character:

$ ./fpm new test^new
<ERROR> the fpm project name must be made of up to 63 ASCII letters,    
        numbers, underscores, or hyphens, and start with a letter.      
STOP 4

We can discuss at a later time if there are some other special characters that we'd want to allow.

…ippets

urbanjost · 2021-01-13T01:32:29Z

Forcing the directory, package, and module name to be the same and to be an allowable Fortran module name was a deliberate decision. Obviously, the module name must conform to these rules, and it was considered that the simplest solution was to enforce them all being consistent. This disallows such options as the package and directory name being UTF-8 and so on so it is defnitely restrictive. I do not know if the discussions are still available, but now that there is auto-discovery it is far less of an issue. The question I have is should there be an option to name the package and the module independently instead of making a special case where dashes are replaced with underscores? If we are going to relax the restriction I think it would be better to leave the default behavior as-is and allow for an optional parameter to name the directory and project. I do not know if UTF-8 or other characters are allowed or easily supported in all the places the name might appear, but I believe toml-f supports UTF-8. So if we allowed any arbitrary name would it cause issues with git, github, gitlab, the fpm repository and/or toml files ?

awvwgk · 2021-01-13T08:16:47Z

I agree that we might have to enforce some restrictions on the project name (e.g. I would consider newlines always invalid), but requiring it to be a valid Fortran identifier seems harsh. Allowing more freedom in fpm-new is very welcome from my side (in fact all my fpm packages but one are using hyphens in the project name).

I like the idea to just underscorify everything we can't use in Fortran, in case we encounter problems with the names of the targets, archive, directories a similar approach could be used as well. The limitations of the programming language shouldn't be imposed on the user in my opinion.

Also, cargo allows a --name option in the new subcommand to specify the project name independently from the project directory. For our case this doesn't solve the issue with the name of the module but I would consider this a placeholder anyway, so chances are good that it will be renamed.

LKedward · 2021-01-13T08:53:37Z

Thanks @milancurcic, I too use hyphens commonly in package names and welcome this change. The proposed solution of replacing with underscores where needed seems reasonable.

With that said, I think the relevant discussion that drives the current behaviour is #153 (Prevent Name Collisions Between Packages) where it was agreed that all module names should be prefixed by the package name. This check is not yet implemented in fpm; when implemented, there will be a small burden on users to ensure modules are correctly prefixed by the fortranised package name. Alternatively, the module namespace prefix could be specified in the manifest separately.

arjenmarkus · 2021-01-13T09:03:03Z

As an aside: we should be thinking about the support for UTF-8. That is a rather tricky topic, as it may well depend on what the compilers allow (think of file names). Note: My remark is not to complicate matters for fpm, just to remind us of the topic in general. Op wo 13 jan. 2021 om 09:53 schreef Laurence Kedward < notifications@github.com>:

…

Thanks @milancurcic <https://github.com/milancurcic>, I too use hyphens commonly in package names and welcome this change. The proposed solution of replacing with underscores where needed seems reasonable. With that said, I think the relevant discussion that drives the current behaviour is #153 <#153> (Prevent Name Collisions Between Packages) where it was agreed that all module names should be prefixed by the package name. This check is not yet implemented in *fpm*; when implemented, there will be a small burden on users to ensure modules are correctly prefixed by the fortranised package name. Alternatively, the module namespace prefix could be specified in the manifest separately. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#337 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN6YR7SCYPKAPV6PIIPXCDSZVNSDANCNFSM4V74FQJQ> .

awvwgk · 2021-01-13T09:05:29Z

For UTF-8 support I will link to the upstream issue at TOML-Fortran (toml-f/toml-f#3) as this will become relevant.

awvwgk

I'm much in favor of this change. We should keep the artificial constraints on our users as little as possible.

LKedward

Changes look good to me, thanks 👍

milancurcic · 2021-01-13T17:17:46Z

With this PR, I didn't mean to ignore #153, which I still think is important. But I forgot about it in the moment.

When fpm checks that all module names do begin with the project name, the change in this PR will require an extra clause explaining the restriction to the user. For example:

Fpm requires that the names of all modules in your package begin with the package name. If your package name contains hyphens (-), the module names should have underscores (_) in place of hyphens.

or similar. So there is a bit of burden that is being put on the user with this. On the other side, with this PR fpm new already generates correct module names with underscores instead of hyphens. Further, when the #153 check is implemented, fpm will be able to tell the user exactly how to prefix the module names. For example, if package name is "my-fortran-lib", fpm can report something like:

<ERROR> All module names in this package must begin with "my_fortran_lib".

urbanjost · 2021-01-14T01:11:15Z

Even now nothing obvious breaks if the directory is renamed, as I have done myself several times so I would prefer a second parameter for naming the directory to a non-default value, but at least for now this is satisfactory for the specific case of a dash.
I can picture where the user might want the entire directory name to be a UTF8 string which could produce a project name of something like "___" so in the long term I think this will need revisited, but during the original discussion I think the conclusion was something along the lines "of a consistent name is simplest and most intuitive, let us go with the restriction and revisit if if there are complaints", which, lo and behold is the case so lets go with this as it covers the most common case and appears to be commonly desired. If there are requests for other changes I think the additional option would be cleaner, but this has been the only request so far.

I believe it is telling that other package managers appear to address this with an additional option, but apparently in the reverse order of precedence I would suggest (the name always names the directory and an alternate project name is allowed for). After trying it that might be better, as it allows for things like fpm . --backfill. So in the future if the name on "new" allows for any name, but if it is required you supply a "Fortran-acceptable" name as well as the primary project name if the directory name is not one I would go with that.

fpm new ANY_DIRECTORY_NAME --name PACKAGE_NAME

appears to be how cargo handles this. I am wondering if anyone using cargo could see if PACKAGE_NAME is required with unusual directory names or if there are any other restrictions. The documentation I read does not mention it, but I wonder what would something like "cargo --new ___" or "cargo --new @" do. And right now, what about "fpm new ./---" ? Obviously unlikely a Linux user in particular would use that particular name, but non-ASCII characters could produce something as bad.

milancurcic · 2021-01-14T15:39:56Z

I think there would be value in a CLI parameter to specify a directory name different from the project name. I have projects like this. For example, the repo and directory are called "datetime-fortran" and "functional-fortran" (to help them stand out on GitHub), but the fpm project names (and modules) are called "datetime" and "functional" because "fortran" is redundant in the context of fpm.

I can picture where the user might want the entire directory name to be a UTF8 string which could produce a project name of something like "___" so in the long term I think this will need revisited

No, currently only hyphen is converted to an underscore. I don't think this should expand to all special ASCII characters and definitely not all Unicode. If the restriction is lifted, it should be on a character by character basis and motivated by user requests.

And right now, what about "fpm new ./---" ?

This will error out because you need to start it with a letter. fpm new a--- should work with this PR but I didn't test it. I doubt this will be a common use of fpm new.

Thank you all for the feedback, I will go ahead and merge.

milancurcic added 5 commits January 12, 2021 17:01

add function to replace characters in strings

7af51a6

add function to replace special characters to undercores

2cb994d

allow hypen in fpm new and update the error message

75e870f

make sure bname is a valid fortran name when inserting into source sn…

efe54f4

…ippets

update error message

27234a1

milancurcic requested review from LKedward, awvwgk and urbanjost January 12, 2021 22:20

milancurcic added 2 commits January 12, 2021 17:35

flip the no-no test to test-with-hyphens

dc1adcf

update the help message for fpm new

838ba4c

awvwgk approved these changes Jan 13, 2021

View reviewed changes

LKedward approved these changes Jan 13, 2021

View reviewed changes

urbanjost approved these changes Jan 14, 2021

View reviewed changes

milancurcic merged commit c485357 into fortran-lang:master Jan 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow hyphens in fpm project names in "fpm new" #337

Allow hyphens in fpm project names in "fpm new" #337

Allow hyphens in fpm project names in "fpm new" #337

Allow hyphens in fpm project names in "fpm new" #337

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment