feat: add Actor exit_process option

[vdusek]

Description

• Introduced a new exit_process option for the Actor class.
• Named the option exit_process (instead of exit) to avoid shadowing Python's built-in names.
• Set reasonable defaults: False for IPython, Pytest, and Scrapy environments, and True for all other cases.
• Updated the test_actor_logs_messages_correctly test accordingly.

Issues

• Closes: Refactor sys.exit handling for tests in Actor #396
• Closes: Relying on SCRAPY_SETTINGS_MODULE isn't sufficient #401

Tests

• Tests are passing, including the Scrapy integration test.

[vdusek]

Hi @honzajavorek, what were your suggestions for utilizing the Scrapy's get_project_settings?

[B4nan]

Named the option call_exit (instead of exit) to avoid shadowing Python's built-in names.

Is this really necessary? I would rather keep the same naming as in the JS version. I recall this argument from another PR, where @janbuchar disagreed with the shadowing being necessary?

[vdusek]

Is this really necessary? I would rather keep the same naming as in the JS version. I recall this argument from another PR, where @janbuchar disagreed with the shadowing being necessary?

Not strictly necessary, just good practice and keeping the linter happy. However, the built-in exit is primarily for interactive use, so it's not that big deal, and keeping argument names consistent is a good argument (although the Actor interfaces differ quite a lot just because of the context managers in Python). Any thoughts, @Pijukatel? Honza is already tagged.

[Pijukatel]

Is this really necessary? I would rather keep the same naming as in the JS version. I recall this argument from another PR, where @janbuchar disagreed with the shadowing being necessary?

Not strictly necessary, just good practice and keeping the linter happy. However, the built-in exit is primarily for interactive use, so it's not that big deal, and keeping argument names consistent is a good argument (although the Actor interfaces differ quite a lot just because of the context managers in Python). Any thoughts, @Pijukatel? Honza is already tagged.

I am leaning towards call_exit as it is still quite similar to JS version, only adapted to Python world, but I do not have strong opinion about this and I am quite ok with also having exit + ignore.

[janbuchar]

Is this really necessary? I would rather keep the same naming as in the JS version. I recall this argument from another PR, where @janbuchar disagreed with the shadowing being necessary?

Not strictly necessary, just good practice and keeping the linter happy. However, the built-in exit is primarily for interactive use, so it's not that big deal, and keeping argument names consistent is a good argument (although the Actor interfaces differ quite a lot just because of the context managers in Python). Any thoughts, @Pijukatel? Honza is already tagged.

In general, I'm pro-shadowing and will always argue in favor of shadowing obscure Python builtins 😁 However, exit on its own is not exactly self-explanatory as a parameter name. call_exit might work better, but I feel that an even better name exists... Something like exit_process maybe?

[B4nan]

I can't help myself, but the call_ prefix sounds really weird to me.

[vdusek]

I have no problem changing to e.g. exit_process as was suggested.

[B4nan]

Let's go with exit_process, that sounds much better.

[Mantisus]

LGTM

[honzajavorek]

LGTM.

My idea was to use get_project_settings() instead of checking the env var, because checking one env var isn't sufficient to detect we're inside a Scrapy project. But I didn't experiment with get_project_settings() enough to see whether it has side effects, or whether it's actually possible to easily distinguish settings created from a project and from a "no project".

This change detects Scrapy by checking whether it's installed, which might not be perfect (perhaps I could use something from Scrapy, but not the whole thing? how many scrapers like that exists? probably not many 🤔 ), but the most important change is that if I realize the SDK doesn't do a great job in detecting my particular use case, I can now explicitly say "please do not call sys.exit()", no bizarre hacks needed. Thanks!

This closes #389 as well.

[vdusek]

Thanks, @honzajavorek. I tried using get_project_settings, but I didn't find anything useful for detecting a Scrapy project there. So I stayed with simply trying to import the scrapy package. This can lead to false positives, of course, but it's probably still improvement over the current approach.