Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node type supported #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

gliush
Copy link

@gliush 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.

@kostis
Copy link
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 '[email protected]', 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
Copy link
Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants