Use iskeyword and str.isidentifier for "is FQCN"

[webknjaz]

SUMMARY

This is extracted from #72591.

ISSUE TYPE

• Refactoring Pull Request

COMPONENT NAME

N/A

ADDITIONAL INFORMATION

N/A

N/A

[sivel]

We already have an isidentifier function in ansible. Located at ansible.utils.vars.isidentifier

[webknjaz]

We already have an isidentifier function in ansible. Located at ansible.utils.vars.isidentifier

I've taken a prompt look and it seems that this helper doesn't replicate the stdlib behavior so I'm not sure if we want to reuse it for FQCNs.

[nitzmahone]

I'm all for tightening this up to delegate to stdlib, but a few questions:

• why expose this as public API (and esp in module_utils, since modules should never need it)?
• should we just wait for 2.12 so we don't need the 2.x fallback?
• if we are doing for 2.11, the regex needs to be compiled- this gets called A LOT

[webknjaz]

@nitzmahone

• why expose this as public API (and esp in module_utils, since modules should never need it)?

Honestly, I wasn't sure where to put it, just wanted a separate compat module. The dependency resolver rewrite (#72591) will need it, ideally through calling is_valid_collection_name, though. Any suggestions?

• should we just wait for 2.12 so we don't need the 2.x fallback?

I wanted to move it out of #72591 so I thought it'd be better to do it sooner than later.

• if we are doing for 2.11, the regex needs to be compiled- this gets called A LOT

This is probably unnecessary — it's already cached internally: https://github.com/python/cpython/blob/c92cd0f/Lib/re.py#L285 / https://github.com/python/cpython/blob/2.7/Lib/re.py#L234-L241.

[nitzmahone]

Unless you've got an immediate concrete need for it independently of is_valid_collection_name, I'd rather keep all that inside _collection_finder for now rather than making a whole new module_utils compat module "just in case". The compile should also definitely be explicit, since the caching is an implementation detail (that's also dependent on the flags). If it's already cached, great, compile will no-op, but otherwise we want to make darn sure it's compiled.

[mattclay]

There's no guarantee the compiled pattern will remain in the cache. The docs state:

The compiled versions of the most recent patterns passed to re.compile() and the module-level matching functions are cached, so programs that use only a few regular expressions at a time needn’t worry about compiling regular expressions.

Looking at the source for both CPython 2.7 and 3.9, the LRU cache size is fixed at 512 entries. So while it's likely to stay cached, we shouldn't assume that.

[nitzmahone]

@webknjaz if you don't want to hold up #72591 for this, no worries, but I do like the overall change and agree that the code in galaxy-cli should share the impl with the loader, so if #72591 gets merged first, maybe just add the impl-sharing change to galaxy-cli here?

[webknjaz]

@nitzmahone @mattclay I moved the shim into _collection_finder and made it use re.compile().

[s-hertel]

This could use a changelog. We might also want to document this in the developing_collections page or improve error messages to give a better clue about what might be wrong.

ansible-galaxy:

ERROR! Invalid collection name 'def.coll', name must be in the format <namespace>.<collection>. 
Please make sure namespace and collection name contains characters from [a-zA-Z0-9_] only.

Using a now-invalid collection:

[WARNING]: errors were encountered during the plugin load for def.coll.my_module: ['invalid collection name (must be of the form namespace.collection): def.coll']

[webknjaz]

Fair enough. But I think that changing the error message should go to a separate PR. The resolver already uses this code in one place anyway...

[s-hertel]

Yeah, but in a very limited way. You could initialize + build + install the tarfile + use it, and it would work. I'm fine with changing the error later though.