Pass program name to QApplication contructor

[hakonhagland]

QCommandLineParser needs QApplication to be initialized with a list with at least one argument: The name of the program.

See #514 (comment) for an example.

Closes #483.

[nicoddemus]

Is this related to #483 somehow?

[The-Compiler]

As mentioned in #483, this should be sys.argv[0] instead. The QApplication.applicationName() isn't necessarily the same thing.

[The-Compiler]

Is this related to #483 somehow?

Looks like it is, and should probably close that issue? This definitely needs a changelog entry and a test as well though.

[hakonhagland]

this should be sys.argv[0] instead. The QApplication.applicationName() isn't necessarily the same thing.

Right, but sys.argv[0] will be something like /home/hakon/.pyenv/versions/3.10.4/bin/pytest if the test is run with pytest, right?

[The-Compiler]

True, but I'd argue that is the correct value. The whole point of sys.argv[0] is for your code to know via which command it was invoked, and I don't think we should mask that. No strong feelings on it though, I can see an argument for either side now that you mention it.

[hakonhagland]

I can see an argument for either side now that you mention it.

@The-Compiler OK fine! I'll then keep the original version for now, if it is ok?

This definitely needs a changelog entry and a test as well though.

Added a unit test.

[hakonhagland]

This definitely needs a changelog entry

@The-Compiler Should I add an entry in the changelog in this PR, or should it be a separate PR?

[nicoddemus]

@The-Compiler Should I add an entry in the changelog in this PR, or should it be a separate PR?

Same PR, thanks!

[hakonhagland]

@nicoddemus In CHANGELOG.rst can I use for example :ref:`qapp fixture<setting-qapp-name>` to cross reference a section in the documentation? It does not seem to work, I get ERROR CHANGELOG.rst:4 Unknown interpreted text role "ref" when compiling.

[hakonhagland]

It does not seem to work

I think it does work, but the linter

pytest-qt/.pre-commit-config.yaml

Line 45 in da33817

entry: rst-lint --encoding utf-8

does not see the whole picture?

[hakonhagland]

but the linter does not see the whole picture?

Is it possible to make the linter somehow ignore this line with :ref: ?

[nicoddemus]

@hakonhagland hmm I don't think it is possible to configure the linter that way, perhaps we can exclude CHANGELOG.rst from the linter instead?

files: ^(CHANGELOG.rst|HOWTORELEASE.rst|README.rst)$

Let's remove CHANGELOG.rst from there, the changelog file is included into the docs, so Sphinx will check that for us already.

[hakonhagland]

Let's remove CHANGELOG.rst from there

Ok done! Please review last commit for changelog update

[nicoddemus]

We probably won't have an immediate release, but even so the process is to set version/date at the time of release. 👍

Suggested change

	4.3.0 (2023-09-20)
	------------------
	UNRELEASED
	----------

[nicoddemus]

Suggested change

	# testdir.makeini(
	# """
	# [pytest]
	# qt_qapp_name = frobnicator
	# """
	# )

[nicoddemus]

@The-Compiler OK fine! I'll then keep the original version for now, if it is ok?

Do you have a strong opinion on this @hakonhagland?

[hakonhagland]

Do you have a strong opinion on this

Do you mean whether to use sys.argv[0] or qt_qapp_name from pytest.ini as the default value for qapp_args ?

[nicoddemus]

Do you mean whether to use sys.argv[0] or qt_qapp_name from pytest.ini as the default value for qapp_args ?

Yes, I'm kind leaning towards @The-Compiler 's original opinion of using sys.argv[0]: seems correct to use /path/to/pytest in that case given we are running the tests, not the actual application.

But only if you do not have a strong opinion on this.

[hakonhagland]

seems correct to use /path/to/pytest

I think the QApplication should not depend on the location of the pytest script in any way. So we should not pass that to QApplication constructor. But the test scripts test_*.py might depend on that, but I have not come up with a good example of this yet. If a test script needs to know the location of the pytest script it should be able to recover that by accessing sys.argv[0] directly right?

[nicoddemus]

I think the QApplication should not depend on the location of the pytest script in any way.

Hmm not sure what you mean, QApplication itself does not care what the first argument is (it just cares if there are no arguments).

But the test scripts test_*.py might depend on that, but I have not come up with a good example of this yet. If a test script needs to know the location of the pytest script it should be able to recover that by accessing sys.argv[0] directly right?

Not sure about what what some tests might need hypothetically, can't think of a situation where someone wants to check the first argument passed to QApplication.

Another point is that the the recommendation in production code is to pass sys.argv, not an "application name + arguments".

[hakonhagland]

the recommendation in production code is to pass sys.argv, not an "application name + arguments".

@nicoddemus I think this is for code running normally, not for code being executed by another program like pytest. Therefore it will be safe and also the correct approach to pass application name instead of the location of the pytest script (that is: sys.argv[0])

[nicoddemus]

@nicoddemus I think this is for code running normally, not for code being executed by another program like pytest. Therefore it will be safe and also the correct approach to pass application name instead of the location of the pytest script (that is: sys.argv[0])

I'm not entirely convinced but TBH I'm not sure this will really matter much in the end, so I'm fine with we going in that direction. 👍

Letting @The-Compiler do another round of review/merge. 👍

Thanks again @hakonhagland for the contribution!

[The-Compiler]

Looks good all in all! Not sure what's up with the failing CI environments, and no time right now to dig into that.

[nicoddemus]

Looks good all in all! Not sure what's up with the failing CI environments, and no time right now to dig into that.

They are all related to pyside6 and unrelated to this PR, except for the pre-commit job:

tests/test_wait_signal.py:620:16: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`

Just pushed a fix to that one.

[nicoddemus]

Thanks @hakonhagland!