Crawly custom settings

[oltarasenko]

1. Extend Crawly.Spider behaviour with otional custom_settings
2. Add utility function to extract settings from a global config
   or spider's custom settings

[Ziinc]

I think using the term custom_settings is not really intuitive of what it actually does.
Essentially, we are overriding certain default settings in config.exs for only this specific spider.
I think using override_settings might be better.

[Ziinc]

I think we should also provide a macro to make simple overrides easy (for some syntactic sugar) , like in my example in the related issue, to get ride of some of the boilerplate. Though this should probably be split to a separate issue.

[Ziinc]

Also add in some tests for the following situations:

• user declares callback but doesn't return a specific option.
• user declares callback that returns nothing or wrong type

[oltarasenko]

@Ziinc thanks for comments. I was not addressing them yet, as I was in the middle of more massive changes already. Will have a closer look soon!

[oltarasenko]

@Ziinc can you please have another glance on it?

[Ziinc]

I think an additional test where no global setting is defined, and no spider setting is defined, should throw an error-reason tuple.

This would be for developer's debugging convenience

[oltarasenko]

Well currently Crawly is built with an idea that it should run even without settings. We have default fallback functions for most or all settings.

This idea can be discussed, but lets maybe do it in a separate issue/pr

[Ziinc]

Ok, was thinking along the lines of when devs create their own pipelines/middlewares and want to define/get their own settings.

[oltarasenko]

Otherwise, it still should be possible

[Ziinc]

should we pass the global config as first arg to the callback? I think it might be beneficial, for example, adjusting pipelines/middlewares on the fly.

[oltarasenko]

Could you please show an example? In general, I don't understand the plan yet. However, if you can show samples I am happy to include it.

Otherwise, if it's private we can't override it, right?

[Ziinc]

Overall looks great 👍 , just some minor changes. The docs on how to access settings in pipelines/middlewares will need to be updated too

[oltarasenko]

@Ziinc I have addressed most (if not all) comments you have made. Could we make another round of the review please :)?

[Ziinc]

is it necessary to list this? all settings are overridable, from my understanding.

[Ziinc]

Ok, was thinking along the lines of when devs create their own pipelines/middlewares and want to define/get their own settings.

[Ziinc]

For example:

def get_settings(setting_name, spider_name \\ nil , defualt \\ nil) do
    global = Application.get_env(:crawly, setting_name, default)
    overrides = cond do
        function_exported?(spider_name, :override_settings, 1) when is_atom(spider_name) ->
            spider_name.override_settings(global)
        function_exported?(spider_name, :override_settings, 0) when is_atom(spider_name) ->
            spider_name.override_settings()
        true -> 
           []
    end
    merged = Keyword.merge(global, overrides)
    Keyword.get(merged, setting_name, default)
end

Allows for more advanced logic in the override function, and the user doesn't have to do a Application.get_env in the spider module.

[oltarasenko]

I am not quite sure about it. Lets maybe discuss it in a separate issue.

[oltarasenko]

@Ziinc I have addressed new code review comments. Let's have another glance. I would suggest discussing all more complex capabilities of custom settings separately, as this PR is quite large already. Also maybe we can do it as a part of callbacks related discussion. (Maybe it's time when we will convert Spider to macro-based definition)

[Ziinc]

Looks good

[Ziinc]

🎉🎉🎉🎉