Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion for subdomain overrides #52

Closed
BurnzZ opened this issue Aug 9, 2021 · 2 comments
Closed

Discussion for subdomain overrides #52

BurnzZ opened this issue Aug 9, 2021 · 2 comments

Comments

@BurnzZ
Copy link
Contributor

BurnzZ commented Aug 9, 2021

Background

Currently, the @override_for decorator only works for top-level domains. However, there are some cases wherein a site has multiple subdomains, usually per country (where each has a slightly different layout). This is due to the current setup of get_domain() (ref).

The problem lies in the following workaround due to limitations of the current @override_for decorator interface:

@override_for("example.com", AutoExtractArticleListData)
@attr.s(auto_attribs=True)
class ExampleArticleListPage(AutoExtractItemWebPage):
    article_list_data: AutoExtractArticleListData

    def to_item(self) -> ArticleList:
    	article_list = self.article_list_data.to_item()

    	if "uk.example.com" in self.url:
    		return self._handle_uk(article_list)

    	elif "au.example.com" in self.url:
    		return self._handle_uk(article_list)

    	elif "fr.example.com" in self.url:
    		return self._handle_fr(article_list)

    	return article_list

Objectives

I'm wondering if it's worth it to update the current interface with something like this?

@override_for("example.com", AutoExtractArticleListData, subdomain="uk")
@override_for("example.com", AutoExtractArticleListData, subdomain="au")
@override_for("example.com", AutoExtractArticleListData, subdomain="fr")

Depending on the situation, this provides another option on how to structure the collection of PageObjects in the project.

@ivanprado
Copy link
Contributor

I've proposed a change to default overrides configuration that allows configuring overrides for subdomains among other things. #53 .

Once merged, it will allow:

@override_for("uk.example.com", AutoExtractArticleListData)
@override_for("au.example.com", AutoExtractArticleListData)
@override_for("fr.example.com", AutoExtractArticleListData)

It will allow other interesting combinations like:

@override_for("example.com", AutoExtractArticleListData)
@override_for("example.com/deals", AutoExtractArticleListData)

or

@override_for("example.com", AutoExtractArticleListData)
@override_for("ie.example.com", AutoExtractArticleListData)

Feel free to review the PR if you want and have the time: comments are always welcome :-)

@BurnzZ
Copy link
Contributor Author

BurnzZ commented Mar 16, 2022

Closing this issue as obsolete since the new PR in #56 will be able to handle subdomain overrides as it leverages the functionalities of https://github.com/zytedata/url-matcher.

@BurnzZ BurnzZ closed this as completed Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants