Unexpected behaviour of snews_pt set-name with no argument

[Sheshuk]

Hi, I'm going through the quickstart instructions, and see a minor problem with the command.

Observed behavior

When I run the command:

$ snews_pt set-name

I get

You are XENONnT
Your detector name is set to be: XENONnT

which I'm not :)

Expected behavior

From the quickstart I read but if executed without an argument, it will display the accepted detector names and request an input based on the index of your detector, so I expected the command to let me choose.

The CLI documentation also doesn't describe what happens without the command:

$ snews_pt set-name --help                                                                                                                                                                                                                                   2 ↵  
Usage: snews_pt set-name [OPTIONS]

  Set your detectors name

Options:
  -n, --name TEXT  Set the detectors name  [default: (TEST)]
  --help           Show this message and exit.

Suggested solutions

We need to either:

1. Fix the command behavior: if run with no name specified, show the allowed experiment names list. Or just print the help message

2. Fix the documentation

[KaraMelih]

Hi @Sheshuk thanks for pointing this out!
I guess you also run some other functions like run-scenarios which sets a detector name on the go.

The functionality we thought was that a user only needs to specify their name once. Which replaces the default detector name here;

SNEWS_Publishing_Tools/snews_pt/auxiliary/test-config.env

Lines 1 to 2 in f9b990a

	DETECTOR_NAME='TEST'
	HAS_NAME_CHANGED='0'

and from then on, it always fetches this name. Now the problem is, during testing, test scenarios are need to be sent with different experiment names, which replaces the default name at each test, and the last name sticks there. We need to think of a better way to circumvent this.

[Sheshuk]

Hm, I don't remember what I did previously on this machine with snews_pt, but here's what I did this time:

#1. Create the new virtual environment
python -m venv venv_dev
source venv_dev/bin/activate
#2. Install the snews_pt package
pip install snews_pt
#3. Added my credentials 
hop auth add
#4. Tried to set the name
snews_pt set-name

So it looks like this last experiment is stored somewhere outside of the current python environment?

[Sheshuk]

Now the problem is, during testing, test scenarios are need to be sent with different experiment names, which replaces the default name at each test, and the last name sticks there. We need to think of a better way to circumvent this.

Yes, looks like this is what happened. Making a message with SNEWSMessageBuilder(detector_name='KamLAND', ...) changes the name of the detector in that file, so all the future tests will call you KamLAND.

I think it is a serious problem: especially since the Firedrills/Publish section gives us explicit example of changing this.

IMO The MessageBuilder should do exactly one thing: build the message (that's what the user expects), and the fact that it changes some state underneath - a global configuration - is totally unexpected and unwanted.

What is the use case of setting the detector_name in the MessageBuilder class? Maybe we should just get rid of this option? And all the name changes should be done through the set_name)

[KaraMelih]

I think you are right, and this should be fixed.
Initially, I implemented it this way thinking that it would be convenient for the user to specify their names wherever they want. My idea was, once we go to production, people will never try using the names of the other detectors. However, you are right, there are examples and test scenarios that suggests/uses different names which indeed changes a global configuration.

If you like, start a PR, otherwise, I'll try to get back to this in some time.

[habig]

The effect of this is that run_scenarios resets the detector name default, when it shouldn't. Caused confusion in today's testing session.

I like the idea that snews_pt set-name with no arguments returns what your name is currently, that's a useful thing.

Also the TEST detector should be a va;id detector name for at the very least testing connections. Maybe this should be a separate issue?