Assert that field names are not valid Python attributes

[nilodna]

Fixes #2101

[VeckoTheGecko]

Thanks for the PR! Comments below. Does the test (test_variable_invalid_init) at

Parcels/tests/test_particle.py

Lines 30 to 32 in 3a05d61

	for name in ["a b", "123", "while"]:
	with pytest.raises(ValueError, match="Particle variable has to be a valid Python variable name. Got "):
	Variable(name)

as well as the test test_fieldset_add_constant in test_fieldset.py show how we can make a test test_fieldset_add_constant_invalid_name for testing this? You can put that new test just below test_fieldset_add_constant

Also, you can edit the test in test_field_init_param_types after the line

with pytest.raises(ValueError, match="Expected `name` to be a string"):

to add a part that tests this bahaviour.

Hope that helps!

[VeckoTheGecko]

I think including the kwlist error message in the output everytime is perhaps excessive - try the following?

Suggested change

	def _assert_valid_python_varname(name):
	if name.isidentifier() and not iskeyword(name):
	return
	raise ValueError(
	f"Received invalid Python variable name {name!r}. Avoid using the following names: {', '.join(kwlist)}"
	)
	def _assert_valid_python_varname(name):
	if name.isidentifier() and not iskeyword(name):
	return
	msg = f"Received invalid Python variable name {name!r}. "
	if name in kwlist:
	msg += f"HINT: Avoid using the following names: {', '.join(kwlist)}"
	raise ValueError(msg)

[nilodna]

I didn't pushed this, but I wrote a new function trying to keep the ValueError separated, mainly because some tests are failing in this part (when a variable name is 'U (A grid)' in the ```test_fieldset.py``, for instance).

def _assert_valid_python_varname(name):
    try:
        if name.isidentifier():
            if not iskeyword(name):
                return
            raise ValueError(f"Received invalid Python variable name {name!r}: it is a reserved keyword. Avoid using the following names: {', '.join(kwlist)}")
        else:
            raise ValueError(f"Received invalid Python variable name {name!r}: not a valid identifier.")
    except Exception as e:
        raise ValueError(f"Error validating variable name {name!r}: {e}")

What do you think?

[VeckoTheGecko]

I wrote a new function trying to keep the ValueError separated

I'm not sure what you mean by this - the code that you send looks to raise ValueErrors in both cases, just with different wording?

because some tests are failing in this part (when a variable name is 'U (A grid)' in the ```test_fieldset.py``, for instance).

Yep! I saw those tests as well. Those tests actually will need to be updated (U (A grid) shouldn't be a valid name for a field - so that's why they're now failing :) . We can't just use the dataarray name like that for the field name - we need to somehow choose other names for those fields in that test).

Hope that makes sense

[nilodna]

I wrote a new function trying to keep the ValueError separated

I'm not sure what you mean by this - the code that you send looks to raise ValueErrors in both cases, just with different wording?

I’ve updated the PR with what I think might be the best way to organize the function. I tried to give separate instructions, that way user can solve the ValueErrors by their own. What do you think?

because some tests are failing in this part (when a variable name is 'U (A grid)' in the ```test_fieldset.py``, for instance).

Yep! I saw those tests as well. Those tests actually will need to be updated (U (A grid) shouldn't be a valid name for a field - so that's why they're now failing :) . We can't just use the dataarray name like that for the field name - we need to somehow choose other names for those fields in that test).

Hope that makes sense

Yeap, make sense. I can change the variable's name in the generic.py script. Perhaps simply changing from U (A grid) to Uagrid solve the issue while keeping the distinction between A and C grid in the test.

[nilodna]

I have finally managed to resolve this issue, and I’d appreciate any feedback on it. I changed the variable names from 'U (A grid)' to 'UAgrid'. It’s not the most elegant solution, but it was a quick workaround to fix the unit test failures, since, as @VeckoTheGecko mentioned, they need to be updated.

[VeckoTheGecko]

HI @nilodna - excuse the late response, I was on leave last week. Thanks for the contribution! I've pushed some minor edits, but we're good to merge now :)