Vrtgs
commented
7 months ago oh yeah msvc is probably 1.75 if that matters too much I'm gonna have to fix that
Closed Vrtgs closed 7 months ago
Vrtgs
commented
7 months ago oh yeah msvc is probably 1.75 if that matters too much I'm gonna have to fix that
stevepryde
commented
7 months ago oh yeah msvc is probably 1.75 if that matters too much I'm gonna have to fix that
Personally I don't mind, but some users might not like it. How much work it is to support versions back 12 months or something like that? Somewhere around 1.69 or 1.70. That's still arbitrary but might be nicer for some users if they're stuck on an older version for whatever reason.
stevepryde
commented
7 months ago …and a bunch of kinda nit-picky changes to docs (including typo fixes)
I ran the tests this time, I learned, but this time I only ran chrome, because I just could not get the gekodriver to work properly
What OS? Wondering if there's any documentation updates we can add to fix this. https://stevepryde.github.io/thirtyfour/contributing/testing.html
I am on Windows WSL but I've tried it on Linux previously. On Ubuntu you can just install the firefox snap and then run geckodriver from a terminal. Then in another terminal/tab you can run the tests using THIRTYFOUR_BROWSER=firefox cargo test.
Vrtgs
commented
7 months ago oh yeah msvc is probably 1.75 if that matters too much I'm gonna have to fix that
Personally I don't mind, but some users might not like it. How much work it is to support versions back 12 months or something like that? Somewhere around 1.69 or 1.70. That's still arbitrary but might be nicer for some users if they're stuck on an older version for whatever reason.
I think we can do that by just adding #[async_trait] to thirtyfour::components::wrapper::resolver::sealed::Resolve
Vrtgs
commented
7 months ago …and a bunch of kinda nit-picky changes to docs (including typo fixes) I ran the tests this time, I learned, but this time I only ran chrome, because I just could not get the gekodriver to work properly
What OS? Wondering if there's any documentation updates we can add to fix this. stevepryde.github.io/thirtyfour/contributing/testing.html
I am on Windows WSL but I've tried it on Linux previously. On Ubuntu you can just install the firefox snap and then run
geckodriverfrom a terminal. Then in another terminal/tab you can run the tests usingTHIRTYFOUR_BROWSER=firefox cargo test.
running geckodriver on the terminal displays a "Machine type Mismatch"
codecov-commenter
commented
7 months ago Attention: Patch coverage is 37.22826% with 231 lines in your changes are missing coverage. Please review.
Project coverage is 41.31%. Comparing base (
f2b9a13) to head (47a11ef).:exclamation: Current head 47a11ef differs from pull request most recent head 45cbafc. Consider uploading reports for the commit 45cbafc to get more accurate results
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Vrtgs
commented
7 months ago I'm curious to know why you felt memory usage was an issue, especially for a web browser automation crate. What is your use-case? Neither performance nor memory usage are usually critical for this kind of software
a bit of a dumb reason honestly what sparked my initial snooping in the code was I got mad that I had to wrap everything in Box::new and Box::pin for the resolver, and looked at the code
and honestly the thing that ticked me off the most to go read the entire codebase and do these changes was because I saw you storing Arc<ElementQueryFn
stevepryde
commented
7 months ago I'm curious to know why you felt memory usage was an issue, especially for a web browser automation crate. What is your use-case? Neither performance nor memory usage are usually critical for this kind of software
a bit of a dumb reason honestly what sparked my initial snooping in the code was I got mad that I had to wrap everything in Box::new and Box::pin for the resolver, and looked at the code
and honestly the thing that ticked me off the most to go read the entire codebase and do these changes was because I saw you storing Arc
but ElementQueryFn was Box<dyn ...>, yes again I realize its a bit of a waste to care too much, I ended up taking it as a challenge for the most part, not that it was that bad for performance
Yep I like the changes you made in that area. The predicate code feels a bit nicer now.
stevepryde
commented
7 months ago …and a bunch of kinda nit-picky changes to docs (including typo fixes) I ran the tests this time, I learned, but this time I only ran chrome, because I just could not get the gekodriver to work properly
What OS? Wondering if there's any documentation updates we can add to fix this. stevepryde.github.io/thirtyfour/contributing/testing.html I am on Windows WSL but I've tried it on Linux previously. On Ubuntu you can just install the firefox snap and then run
geckodriverfrom a terminal. Then in another terminal/tab you can run the tests usingTHIRTYFOUR_BROWSER=firefox cargo test.running
geckodriveron the terminal displays a "Machine type Mismatch"
That's odd. What OS? What arch? Are you downloading the correct version of geckodriver?
stevepryde
commented
7 months ago oh yeah msvc is probably 1.75 if that matters too much I'm gonna have to fix that
Personally I don't mind, but some users might not like it. How much work it is to support versions back 12 months or something like that? Somewhere around 1.69 or 1.70. That's still arbitrary but might be nicer for some users if they're stuck on an older version for whatever reason.
I think we can do that by just adding #[async_trait] to thirtyfour::components::wrapper::resolver::sealed::Resolve
I think this is worth doing
stevepryde
commented
7 months ago Once you're done making changes, please provide a summary of the changes and the reasons for them. I feel that these later changes probably should have been a separate PR. Let's not try to change the entire codebase in one commit 😅
Vrtgs
commented
7 months ago Once you're done making changes, please provide a summary of the changes and the reasons for them. I feel that these later changes probably should have been a separate PR. Let's not try to change the entire codebase in one commit 😅
hmm yeah, I should
Vrtgs
commented
7 months ago …and a bunch of kinda nit-picky changes to docs (including typo fixes) I ran the tests this time, I learned, but this time I only ran chrome, because I just could not get the gekodriver to work properly
What OS? Wondering if there's any documentation updates we can add to fix this. stevepryde.github.io/thirtyfour/contributing/testing.html I am on Windows WSL but I've tried it on Linux previously. On Ubuntu you can just install the firefox snap and then run
geckodriverfrom a terminal. Then in another terminal/tab you can run the tests usingTHIRTYFOUR_BROWSER=firefox cargo test.running
geckodriveron the terminal displays a "Machine type Mismatch"That's odd. What OS? What arch? Are you downloading the correct version of geckodriver?
windows x86_64, I downloaded geckodriver-v0.33.0-win64.zip
stevepryde
commented
7 months ago I appreciate the effort you've put in, but dumping a huge code change like this as a PR makes it really difficult to sift through and understand the intent and the implications.
I like the improvements to the predicate functions but a lot of other changes seem to increase the complexity of the codebase without offering a tangible real-world benefit either to the developer or to the runtime outcome. I'd even suggest there's a fair chunk that largely comes down to personal preference. This is the first I've seen anyone recommend Arc<str> instead of String, for example. I can see why you'd care about this for a performance critical application, but this isn't that. Instead many users of the library will probably find it confusing.
I think there's an argument for keeping things familiar and not increasing the complexity unnecessarily.
Vrtgs
commented
7 months ago I appreciate the effort you've put in, but dumping a huge code change like this as a PR makes it really difficult to sift through and understand the intent and the implications.
I like the improvements to the predicate functions but a lot of other changes seem to increase the complexity of the codebase without offering a tangible real-world benefit either to the developer or to the runtime outcome. I'd even suggest there's a fair chunk that largely comes down to personal preference. This is the first I've seen anyone recommend
Arc<str>instead ofString, for example. I can see why you'd care about this for a performance critical application, but this isn't that. Instead many users of the library will probably find it confusing.I think there's an argument for keeping things familiar and not increasing the complexity unnecessarily.
hmm yea I see your point, and I think I really should make a list of all the changes and detail what how and why I did them
as for Arc<str> I think should be used here because we just want an immutable view to a string slice, and as can be see from the code since we clone the string slice a lot I think it would be useful to use an Arc<str> instead of a String, and I mean most of the changes that came with that were not at the site where the string was used, just where it was created and given a type
stevepryde
commented
7 months ago I appreciate the effort you've put in, but dumping a huge code change like this as a PR makes it really difficult to sift through and understand the intent and the implications. I like the improvements to the predicate functions but a lot of other changes seem to increase the complexity of the codebase without offering a tangible real-world benefit either to the developer or to the runtime outcome. I'd even suggest there's a fair chunk that largely comes down to personal preference. This is the first I've seen anyone recommend
Arc<str>instead ofString, for example. I can see why you'd care about this for a performance critical application, but this isn't that. Instead many users of the library will probably find it confusing. I think there's an argument for keeping things familiar and not increasing the complexity unnecessarily.hmm yea I see your point, and I think I really should make a list of all the changes and detail what how and why I did them
as for
Arc<str>I think should be used here because we just want an immutable view to a string slice, and as can be see from the code since we clone the string slice a lot I think it would be useful to use anArc<str>instead of a String, and I mean most of the changes that came with that were not at the site where the string was used, just where it was created and given a type
I understand the reason for the Arc<str>. It's not a bad change. There is some benefit like you said. And I think you did it in a way that doesn't affect how the library is used. It seems to be an internal change only for the most part. So that's fine.
Don't worry about splitting it or anything. Let's just put a freeze on the feature changes and I'll have another look through the code and we can go from there.
If you can summarise the changes with a bullet point each that would be helpful too.
Vrtgs
commented
7 months ago I appreciate the effort you've put in, but dumping a huge code change like this as a PR makes it really difficult to sift through and understand the intent and the implications. I like the improvements to the predicate functions but a lot of other changes seem to increase the complexity of the codebase without offering a tangible real-world benefit either to the developer or to the runtime outcome. I'd even suggest there's a fair chunk that largely comes down to personal preference. This is the first I've seen anyone recommend
Arc<str>instead ofString, for example. I can see why you'd care about this for a performance critical application, but this isn't that. Instead many users of the library will probably find it confusing. I think there's an argument for keeping things familiar and not increasing the complexity unnecessarily.hmm yea I see your point, and I think I really should make a list of all the changes and detail what how and why I did them as for
Arc<str>I think should be used here because we just want an immutable view to a string slice, and as can be see from the code since we clone the string slice a lot I think it would be useful to use anArc<str>instead of a String, and I mean most of the changes that came with that were not at the site where the string was used, just where it was created and given a typeI understand the reason for the
Arc<str>. It's not a bad change. There is some benefit like you said. And I think you did it in a way that doesn't affect how the library is used. It seems to be an internal change only for the most part. So that's fine.Don't worry about splitting it or anything. Let's just put a freeze on the feature changes and I'll have another look through the code and we can go from there.
If you can summarise the changes with a bullet point each that would be helpful too.
here since I'm done with the first part for the first list of major changes you can read through while I write part 2
List of changes:
lets start with thirtyfour-macros, the changes I made were:
as for thirtyfour it self:
remove_arg and args and has_arg from their specific browsers capabilities, and moved it to BrowserCapabilitiesHelper, because they are the exact same function has_arg slightly, to avoid the one extra copy of the argument made to pass into contains, and instead used .iter().any() and checked for equality directlyArc<str> to make the enclosing structs cheaply cloneable, and Arc<str> isn't any different from a normal String, so it shouldn't matter that much from an ergonomic standpoint, as you can use it in basically much the same wayArc<str>tokio's async OnceCell, this makes sure that the query runs only once per time caching, and if it was already initialized there is a fast path that returns the already initialized value, I store this behind an ArcSwap which basically is an Arc that I can also atomically change the value it is pointing to, which allows the resolution of components to be nearly fully lock freevalidate and resolve methods onto those types, instead of duplicating the code again and againArc<str>other changes have either been discussed or are very minor
…and a bunch of kinda nit-picky changes to docs (including typo fixes)
I ran the tests this time, I learned, but this time I only ran chrome, because I just could not get the gekodriver to work properly