More accurate name for the Python versioning preset

[majorz]

In #99 we added support for version updates of Python modules. Although it is more common that the version number string __version__ exists in a Python module's __init__.py file, it is possible a Python module to be in the form of a file (like many in the std lib) and not in the form of a directory. If that is the case, then there is not an __init__.py file, but the file name is equivalent to the module name.

More on that here: PEP 396 -- Module Version Numbers

So it is better to rename the preset to something like pythonModule or python. I lean towards python since this is the standard way version numbers are defined in Python.

Another related improvement is the targetFile argument. If we allow this to be a module directory path as well, then it will automatically mean that it targets __init__.py in that directory. So targetFile can be renamed to target or path, which can accept both directory and file paths. The same can be used in #106 and #105 as well instead of targetDir.

[lurch]

Yeah, and I think other Python projects might just embed their version directly in setup.py ?
Could we add (and document) the python preset and keep initPy as an alias of python (and remove initPy from the docs) so that we don't break backwards compatibility? ping @nghiant2710

I'm not sure if I'd be keen on the idea of having a path option that could refer to a file or a directory with versionist choosing slightly different behaviour in each case - I think it might be clearer to have explicitly separate options for both? I'll have a think about it.
(but I wouldn't be against adding a targetDir also to the python preset, with the code then using ${targetDir}/__init__.py and error-ing if you specify both targetFile and targetDir)

[majorz]

My thinking is that as long as versionist acts in expected and natural way for the particular language, and the behaviour is well documented, things should be fine. It already automatically handles cases like having Cargo.toml with Cargo.lock and without Cargo.lock.

That is to say, we do not have to specify explicitly which Cargo.* file should be processed, e.g. by using targetFile option. So this is all in the same line of thinking.

This as you mentioned will eliminate the need to error out on specifying both targetFile and targetDir.

As for the cases where the version is directly specified in setup.py and other things like that, I am thinking that we should not care about those cases. Let the user either convert his code to what we support as a common approach, or let him implement a custom preset (e.g. in the form of a plugin :D, lol).

Django for example uses a VERSION which is formatted into __version__: https://github.com/django/django/blob/master/django/__init__.py. Such scenario can be handled with a custom preset.

Not sure I expressed myself well, or I am confused, or both.

[lurch]

That is to say, we do not have to specify explicitly which Cargo.* file should be processed, e.g. by using targetFile option. So this is all in the same line of thinking.

Yeah, but OTOH if we had a path option the user might try to set a file on the cargo preset, whereas the cargo preset only works with directories (so only allowing targetDir makes sense). And similarly the updateQuotedVersion preset will only ever work with files, and not with directories (so only allowing targetFile makes sense). All IMHO of course!

Perhaps @jviotti or @jhermsmeier or @hedss or @lekkas would like to share their wisdom? ;)

As for the cases where the version is directly specified in setup.py and other things like that, I am thinking that we should not care about those cases.

Well at the moment we do support that case - the user just needs to specify

  updateVersion: {
    preset: 'initPy',
    targetFile: 'setup.py'
  }

Django for example uses a VERSION which is formatted into version: https://github.com/django/django/blob/master/django/__init__.py. Such scenario can be handled with a custom preset.

Yeah, something as bizarre as that will always need to use a custom preset 😉
After a bit of digging I found https://github.com/django/django/blob/master/django/utils/version.py#L19 which says that Django uses a PEP 440 compatible version, and that's definitely not semver compatible as it allows X.Y version numbers.

[majorz]

Well at the moment we do support that case - the user just needs to specify

Talking about setup.py I checked the spec now. How do you understand the "vice versa" there as I am confused by it?

The version attribute in a classic distutils setup.py file, or the PEP 345 [7] Version metadata field SHOULD be derived from the version field, or vice versa.

Do they mean that the Version metadata field can be used for generating __version__ in __init__.py somehow?

I see your point about targetDir better now, and I agree with your opinion now. Merging targetDir and targetPath will cause too much inconsistency.

There is confusion coming from the fact that targetDir is a general option, while targetFile is too specific to the Python preset.

Maybe they should not share the same prefix (target)? And targetFile should be defined as a relative path to targetDir? (this way it makes perfect sense to specify both options at the same time and the more specific option does not overwrite the general one)

[majorz]

workingDir instead of targetDir - since it is supposed to overwrite cwd? Or dir or directory?

[lurch]

Talking about setup.py I checked the spec now. How do you understand the "vice versa" there as I am confused by it?

Dunno, I haven't read the PEP, and I don't think we should concern ourselves with it (sorry if I caused confusion by not making that clear in my earlier comments). I was just thinking of simple use-cases like https://github.com/RPi-Distro/python-gpiozero/blob/master/setup.py#L25

I see your point about targetDir better now, and I agree with your opinion now. Merging targetDir and targetPath will cause too much inconsistency.

Cool 🙂

And targetFile should be defined as a relative path to targetDir? (this way it makes perfect sense to specify both options at the same time and the more specific option does not overwrite the general one)

Oooh, that's a good idea! 😀 Should we error if targetFile contains any path-separators, so that only these would be allowed:

  updateVersion: {
    preset: 'python',
    targetDir: 'foo/bar'
  }

  updateVersion: {
    preset: 'python',
    targetDir: 'foo/bar',
    targetFile: '__init__.py'
  }

or should we always just join the two together, so that these four would all be equivalent:

  updateVersion: {
    preset: 'python',
    targetDir: 'foo/bar'
  }

  updateVersion: {
    preset: 'python',
    targetFile: 'foo/bar/__init__.py'
  }

  updateVersion: {
    preset: 'python',
    targetDir: 'foo/bar',
    targetFile: '__init__.py'
  }

  updateVersion: {
    preset: 'python',
    targetDir: 'foo',
    targetFile: 'bar/__init__.py'
  }

? (even in the latter more-lenient case, I guess we'd still need to error if targetFile starts with a / ?)

[majorz]

+1 for error when targetFile starts with /.

Now looking at the examples above, targetDir and targetFile look nice together aesthetically. If we leave them with the same prefix, then allowing targetFile to include separator does not feel right (yeah, some of the examples above cause an itchy feeling). I am leaning towards producing an error when it contains separators.

[lurch]

...and just for the sake of completion, these would also be equivalent:

  updateVersion: {
    preset: 'python'
  }

  updateVersion: {
    preset: 'python',
    targetFile: '__init__.py'
  }

  updateVersion: {
    preset: 'python',
    targetDir: '.',
    targetFile: '__init__.py'
  }

  updateVersion: {
    preset: 'python',
    targetDir: '.'
  }

I think we're in agreement now ( 🎉 ) but we should still wait for feedback from @nghiant2710 before working on this.

To maintain backwards-compatibilty, we should probably leave the initPy preset as-is (although I'd still vote for deprecating it and removing it from the documentation) and add all these new features in a separate python preset?

[majorz]

Oh, now that you provided this example (#110 (comment)), it starts to make sense to allow targetFile to contain separators. As this way specifying the workingDir can be avoided when the targetFile is deep inside the working directory.

Also related: since targetFile will be supported in the quoted regex preset, I am starting to wonder whether it should be made to support list of files as well? Or even globs? If it supports multiple files, then we should allow separators in targetFile.

BTW, the replace-in-file package that we use specify the same parameter as files - it accepts one or more files or globs. I wonder if there is some common practice for this in the npm world.

+1 for leaving initPy and deprecating it, but removing it from the documentation.

[majorz]

Thinking about this further, since the most common scenario will be a single file, it will feel odd for naming the argument files as in replace-in-file. We can leave it as is.

[lurch]

Yeah, I'd also be 👎 on supporting multiple files or globs - dealing with multiple files is exactly what #107 is about 😃

So, I guess we have a number of different options:

1. Allow separators in targetFile, but don't allow targetDir and targetFile to be specified at the same time
2. Allow separators in targetFile, and append targetFile to targetDir if it's specified
3. Don't allow separators in targetFile, and append targetFile to targetDir if it's specified
4. Allow separators in targetFile only if targetDir isn't specified
5. Ignore the whole idea of allowing an update-preset to have both a targetDir and a targetFile option
6. Something else?

Disadvantage of 2. is that it allows 'yucky' things like:

  updateVersion: {
    preset: 'python',
    targetDir: 'foo',
    targetFile: 'bar/__init__.py'
  }

Disadvantage of 3. is that it means you need to do:

updateVersion: {
    preset: 'updateQuotedVersion',
    targetDir: 'meta-resin-common/conf/distro/include',
    targetFile: 'resin-os.inc',
    regex: '^DISTRO_VERSION\s+=\s+',

Disadvantage of 4. is that it's slightly more complex logic (which may confuse users?).

Whichever we go for, we should maintain consistent behaviour across all the different update-presets. Pretending for now that #106 #105 #112 and #110 have all been implemented, I think we'd have:

updateVersion preset	parameter	default value	mandatory
npm	baseDir	.	No
cargo	baseDir	.	No
python	baseDir	.	No
python	file	__init__.py	No
quoted	baseDir	.	No
quoted	file	None	Yes
quoted	regex	None	Yes
quoted	regexFlags	empty string	No
initPy	targetFile	__init__.py	No

(obviously if we went for option 1. above, then updateQuotedVersion targetDir wouldn't exist)

EDIT: Updated the table, following discussions below

[majorz]

Yeah, I'd also be 👎 on supporting multiple files or globs - dealing with multiple files is exactly what #107 is about 😃

I would not say #107 resolves multiple files that well. Let's consider the example:

updateVersion: {
    preset: 'updateQuotedVersion',
    targetDir: 'meta-resin-common/conf/distro/include',
    targetFile: 'resin-os.inc',
    regex: '^DISTRO_VERSION\s+=\s+',
}

If two files from the same directory has to be updated with the same preset, then the only option would be to copy and paste the whole thing. If the files are four or five, then it will be even worse.

[majorz]

Support for multiple files can be expanded on top of the same API without the need for breaking changes, so it is not that important at this stage.

[majorz]

I am looking at the parameters name and values above. In general I prefer more concise names when possible. So looking at the example above, I would say I like it more like:

updateVersion: {
    preset: 'quoted',
    workDir: 'meta-resin-common/conf/distro/include',
    file: 'resin-os.inc',
    regex: '^DISTRO_VERSION\s+=\s+',
}

workDir instead of targetDir or just dir, because this is what will be extracted from options and passed into cwd here: npm: (options, cwd, version, callback) => {.

file instead of targetFile, since file is sufficient enough.

What do you think?

EDIT: For this one I do not have that stronger opinion. I guess it is a matter of personal taste only.

[majorz]

2. Allow separators in targetFile, and append targetFile to targetDir if it's specified

I think we should go with the simplest option ^^. Although it has the disadvantage like you showed, this would be totally users fault, as there are better ways to define the same thing. And also that will leave the door open to eventually supporting multiple files in the future.

[lurch]

If two files from the same directory has to be updated with the same preset, then the only option would be to copy and paste the whole thing. If the files are four or five, then it will be even worse.

I'd argue that if there are four or five files in the same directory that all need to be version-bumped at the same time, then that project is probably doing something wrong 😜

With regards to the parameter-names I also don't have strong opinions, so I'm happy to go with your suggestions. Not sure I'm too keen on workDir though - how about baseDir instead? Should we also prevent baseDir from starting with a /, so the eventual target-file is effectively always ${cwd}/${baseDir}/${file} ?
(and like you say, we can always expand file to optionally be an array in future, should that ever be needed).

[majorz]

I like that: ${cwd}/${baseDir}/${file}. baseDir sounds better to me as well.