Skip to content

refactor!: Rename PwPreNavContext to PwPreNavCrawlingContext#827

Merged
vdusek merged 2 commits intomasterfrom
naming-of-pw-prenav-crawl-context
Dec 19, 2024
Merged

refactor!: Rename PwPreNavContext to PwPreNavCrawlingContext#827
vdusek merged 2 commits intomasterfrom
naming-of-pw-prenav-crawl-context

Conversation

@vdusek
Copy link
Copy Markdown
Collaborator

@vdusek vdusek commented Dec 17, 2024 •
edited
Loading

Description

  • Rename PlaywrightPreNavigationContext to PlaywrightPreNavigationCrawlingContext.
  • Of course, this is really long. Do we want to make it shorter, e.g.: PlaywrightPreNavCrawlingContext?
  • Or make shorter all the crawling contexts... e.g. use just CrawlContext, (PlaywrightPreNavCrawlContext)
  • Opinions?

Breaking changes

  • The PlaywrightPreNavigationContext was renamed to PlaywrightPreNavCrawlingContext.

@vdusek vdusek added adhoc Ad-hoc unplanned task added during the sprint. v0.5 labels Dec 17, 2024
@vdusek vdusek self-assigned this Dec 17, 2024
@vdusek vdusek added this to the 105th sprint - Tooling team milestone Dec 17, 2024
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Dec 17, 2024
@janbuchar
Copy link
Copy Markdown
Collaborator

janbuchar commented Dec 18, 2024

I'm not sure if the consistency is worth the BC break. Also, yes, the new name does seem long, and inventing new abbreviations such as CrawlContext doesn't make much sense to me. That's my opinion, feel free to try and change it 😄

@vdusek
Copy link
Copy Markdown
Collaborator Author

vdusek commented Dec 18, 2024

@janbuchar And what about PlaywrightPreNavCrawlingContext? At least a few chars shorter. The issue is that all other "crawling contexts" are consistently named as CrawlingContext, except for this one. Additionally, the term "context" is used for other purposes, such as launch context, browser context, ...

@janbuchar
Copy link
Copy Markdown
Collaborator

janbuchar commented Dec 18, 2024

Is it even crawling if you haven't navigated to the page yet? 🙂 We do have a lot of contexts, but this one doesn't fit with crawling context all that well either...

@vdusek
Copy link
Copy Markdown
Collaborator Author

vdusek commented Dec 18, 2024

True, but the same applies to the basic crawling context 🙂. This was just an idea to straighten things out for now. The PwPreNavCC was written by an external developer, and we (or at least I) overlooked the absence of "crawling" in it.

Pijukatel
Pijukatel reviewed Dec 18, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Pijukatel Pijukatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it should be consistently named. I am split on using abbreviations, I like to make the name shorter, but on the other hand "Explicit is better than implicit".

If the developer will have to constantly look what some abbreviation means, then it is bad. If on the other hand it is natural and people understand it out of the box, then I think it is OK.

PwPreNavCC? Why not if the docstring starts as """PlaywrightPreNavigationCrawlingContext is ...." Even though I would maybe keep at least one full key word in the name to roughly recognize the type on the first glance. For example: PwPreNavCContext

@vdusek
Copy link
Copy Markdown
Collaborator Author

vdusek commented Dec 18, 2024

Agreed. But for this PR and the v0.5 release, let's focus only on resolving the PlaywrightPreNavCrawlingContext issue. The broader discussion about simplifying these names (...CrawlContext, ...CContext, ...Context, etc.) should be postponed for later, it will be more difficult and we will also have to discuss that with JS-guys, as this naming should be aligned.

@vdusek vdusek force-pushed the naming-of-pw-prenav-crawl-context branch from 7c756fd to 81dfe5a Compare December 18, 2024 15:57
@vdusek vdusek requested a review from Pijukatel December 18, 2024 15:57
@vdusek vdusek force-pushed the naming-of-pw-prenav-crawl-context branch from 81dfe5a to 0f4c755 Compare December 18, 2024 15:58
@janbuchar
Copy link
Copy Markdown
Collaborator

janbuchar commented Dec 19, 2024

Still not sure about the benefit of the change, but I won't block it 😁

@vdusek vdusek merged commit 84b61a3 into master Dec 19, 2024
@vdusek vdusek deleted the naming-of-pw-prenav-crawl-context branch December 19, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Pijukatel Pijukatel Pijukatel approved these changes

@janbuchar janbuchar Awaiting requested review from janbuchar

Assignees

@vdusek vdusek

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. v0.5

Projects

None yet

Milestone

105th sprint - Tooling team

Development

Successfully merging this pull request may close these issues.

3 participants

@vdusek @janbuchar @Pijukatel