-
Notifications
You must be signed in to change notification settings - Fork 512
refactor!: Rename PwPreNavContext to PwPreNavCrawlingContext #827
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
Conversation
|
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 |
|
@janbuchar And what about |
|
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... |
|
True, but the same applies to the basic crawling context 🙂. This was just an idea to straighten things out for now. The |
There was a problem hiding this 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
|
Agreed. But for this PR and the v0.5 release, let's focus only on resolving the |
7c756fd to
81dfe5a
Compare
81dfe5a to
0f4c755
Compare
|
Still not sure about the benefit of the change, but I won't block it 😁 |
Description
PlaywrightPreNavigationContexttoPlaywrightPreNavigationCrawlingContext.PlaywrightPreNavCrawlingContext?CrawlContext, (PlaywrightPreNavCrawlContext)Breaking changes
PlaywrightPreNavigationContextwas renamed toPlaywrightPreNavCrawlingContext.