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

extraction chains #458

Merged
merged 2 commits into from
Oct 15, 2024
Merged

extraction chains #458

merged 2 commits into from
Oct 15, 2024

Conversation

MrTpat
Copy link

@MrTpat MrTpat commented Oct 1, 2024

So this introduces a concept of "extraction chains" for our job parsing. Certain issues: (#114) are happening because there are parsing changes necessary when LinkedIn changes something on their end. Instead of changing our parsing code each time, we can add a new parser here when we encounter some new linkedin HTML format.

Shoutout to @pafthinder for his initial correct implementation which I've included here. This fixes THAT issue and fixes issues I had been having myself.

@MrTpat MrTpat changed the title Execution chains extraction chains Oct 1, 2024
@MrTpat
Copy link
Author

MrTpat commented Oct 1, 2024

@feder-cr opened a new PR for V4 as you asked.

@feder-cr
Copy link
Owner

feder-cr commented Oct 2, 2024

@MrTpat please explain better the PR

@MrTpat
Copy link
Author

MrTpat commented Oct 2, 2024

@feder-cr was the description of the original PR not sufficient?

@feder-cr
Copy link
Owner

feder-cr commented Oct 2, 2024

@MrTpat copy and paste here

@MrTpat
Copy link
Author

MrTpat commented Oct 2, 2024

@feder-cr updated.

@MrTpat MrTpat mentioned this pull request Oct 2, 2024
@feder-cr
Copy link
Owner

feder-cr commented Oct 6, 2024

@MrTpat please on V4

@MrTpat
Copy link
Author

MrTpat commented Oct 6, 2024

@feder-cr it is on v4, but the tests are failing. i don't quite have the time to fix the tests, if someone else wants to pick this work up please go ahead!

@MrTpat
Copy link
Author

MrTpat commented Oct 9, 2024

@feder-cr i fixed the tests. can you merge?

@RushiChaganti
Copy link
Contributor

Hey @MrTpat, I’ve gone over the files you modified, but I believe this method will also fail if LinkedIn updates their class names. So, I think neither your approach nor mine is ideal for the program. Let's have a conversation to figure out a better solution. What do you think? No offense intended.

@MrTpat
Copy link
Author

MrTpat commented Oct 13, 2024

@RushiChaganti the idea is that once linkedin updates their class names, someone can add an extraction chain implementation that adheres to that class change.

@feder-cr
Copy link
Owner

@MrTpat very good idea, but please on V4

@MrTpat
Copy link
Author

MrTpat commented Oct 13, 2024

@feder-cr im confused what you mean by "please on V4". are you asking me to rebase?

@MrTpat MrTpat force-pushed the execution-chains branch 2 times, most recently from 981b2c1 to e9efa34 Compare October 13, 2024 17:46
@feder-cr
Copy link
Owner

@MrTpat MrTpat changed the base branch from main to v4 October 14, 2024 22:08
@MrTpat
Copy link
Author

MrTpat commented Oct 14, 2024

@feder-cr done.

@feder-cr feder-cr merged commit dceae26 into feder-cr:v4 Oct 15, 2024
1 check passed
@moudimash99
Copy link

@MrTpat I'm interested did this get published in the end?

@surapuramakhil
Copy link
Contributor

@moudimash99 there are not in release yet. PR to track & participate in reviews providing your feedback.

#813

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

Successfully merging this pull request may close these issues.

5 participants