node type supported #68

Open
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants

gliush commented Apr 25, 2013

I needed a proper test for the module that works with the 'node()' type.
I'm not sure that it's done correctly as I don't know the code good enough. I've got the 'bool()' type as an example for my modification.

btw, I don't quite understand why in a comment in proper_types.erl it's said that 'node' type won't be done.

Ivan Glushkov added some commits Apr 25, 2013

Ivan Glushkov node type supported 36e6c96
Ivan Glushkov typo 0e3c277
Collaborator

kostis commented Apr 28, 2013

The node() type is supposed to represent valid node (i.e. machine) names, that is they are supposed to be atoms of the form 'dilbert@uab.ericsson.se', not random atoms such as 'foo#42b!&!ar'.

So, in most test case scenarios that we envisioned, the generation of valid node names has to be test-environment specific (e.g. the user has to supply a valid list of machine and domain names) in order for the test to be properly executed. Hence, it cannot be totally random.

Is your use-case different than this? Perhaps you can give us some more info why declaring node() as just atom() is something that makes sense in all cases. Otherwise, we cannot merge your pull request.

gliush commented Apr 29, 2013

Agreed with the environment specific nature of node(). But not everybody need it. And moreover absence of any node() generator hurts more, than it's simplest realization.

About my use-case: I have an internal config handler and processor. It doesn't have functions with side-effects, just saving and processing input data. It has several records specifications, in one of them node() type is used. As that recors is used everywhere throughout the module, I can't check_specs any function in my module.
So I don't need any node-specific logic there, an atom generator perfectly fits it.

I had two options for the check_specs to work: either use atom() instead of node() or have a node() generator, semantically equal to atom().
As I want the code to be self-documented, I don't want to use the first one.

We could consider adding more logic to the node() generator.
I'd propose not supporting long node names, as I don't see an easy way to support both short and long node names (I mean generator shouldn't mix them, so the user should somehow specify how what should proper generate, and I don't see how it could be done).

And short node name should be generated with the usual restrictions to the node names.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment