-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
v4 PR big update #813
base: release/v4.0.0
Are you sure you want to change the base?
v4 PR big update #813
Conversation
…e-with-resume-flag
@queukat this PR delta around 1000 loc. total v4 delta would be extremely high, you can't expect to have effective reviews when delta is at this level. I would suggest smaller PR's when you plan to release. The bigger the delta, the harder it becomes to resolve confits, harder for review, harder for even maintaining compatibility. I would suggest doing it as small tasks and having smaller PR's going forward. |
Hi @surapuramakhil, This is a completely new and fully tested release for V4, which was prepared before the release branches and other structures were set up. Ideally, @feder-cr was supposed to merge my previous release with the main branch, so there shouldn’t be any issues. |
@surapuramakhil This is a big update, we can't ignore it, how about adding it to the next merge on the 15th? or to the one on the proimo month? @queukat good work |
@feder-cr this PR is only pointing to v4, v4 has merge conflicts with main that a bigger problem to solve, right now |
@queukat can you resolve the conflict? |
@feder-cr shall we drop v4 branch here? as it is unnecessary since it looks like we will be merging directly it to release branch like any other PR. |
@surapuramakhil yes |
@queukat any update? |
@queukat converting to draft - as it makes unwanted notifications while you make any commits. |
# Conflicts: # tests/test_linkedIn_authenticator.py
Hey @queukat , can you update this? It is hard to understand what you did, what features you did. It looks like an achievement sheet. |
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.
reviewed up to main. You have a lot of lint delta
If you wish for refactoring let's do it completely. let move them to separate files, appropriate packages
``` | ||
python answer_editor.py | ||
``` | ||
Then open a web browser and navigate to `http://localhost:5000` |
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.
can you add UI screenshot?
|
||
## cleanse_answers.py | ||
|
||
This script is designed to clean and sanitize the questions and answers stored in the JSON file. |
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.
can you make sure - duplicate question fix doesn't get effected by this.
for i, item in enumerate(data): | ||
if f'delete_{i}' not in request.form: | ||
if item['type'] == 'radio': | ||
item['answer'] = request.form.get(f'answer_{i}_radio', item['answer']) |
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.
can you also perform cleansing here? so that we don't need to run cleansing script if we edit. or udpate take care of clensing
import json | ||
import re | ||
|
||
def sanitize_text(text: str) -> str: |
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.
can you sanitize while we save
"distance": int, | ||
"company_blacklist": list, | ||
"title_blacklist": list, | ||
"llm_model_type": str, |
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.
line 84, 85 -> they are removed - check base branch
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.
revert this change
logger.error( | ||
f"Invalid value for experience level '{level}'. Expected a boolean (True/False)." | ||
) | ||
raise ConfigError( |
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.
problem is we don't dump all errors at once. They fix error run it again to next validation errors.
raising expception will break for loop
if not isinstance(parameters.get(blacklist), list): | ||
raise ConfigError(f"'{blacklist}' must be a list in config file {config_yaml_path}") | ||
raise ConfigError( |
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.
can we throw config error only once if any of the validation failed? we already have log.error for showing error info
raise ConfigError( | ||
f"Invalid email format in secrets file {secrets_yaml_path}." | ||
) | ||
if not secrets["password"]: |
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.
remove this.
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.
problem is we don't dump all errors at once. They fix error run it again to next validation errors.
Applies here
|
||
if not secrets['llm_api_key']: | ||
raise ConfigError(f"llm_api_key cannot be empty in secrets file {secrets_yaml_path}.") | ||
return secrets['llm_api_key'] | ||
|
||
class FileManager: |
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.
can to take these classes out. as you made lot of refactoring delta, why don't we do it complete.
try: | ||
options = chrome_browser_options() | ||
service = ChromeService(ChromeDriverManager().install()) | ||
return webdriver.Chrome(service=service, options=options) | ||
except Exception as e: | ||
raise RuntimeError(f"Failed to initialize browser: {str(e)}") | ||
|
||
def create_and_run_bot(parameters, llm_api_key): | ||
|
||
def create_and_run_bot(email, password, parameters, llm_api_key): |
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.
Email and password are not needed to provide. bot prompts screen to user for then to enter creds & clear any things like capcha
Overall Improvement Summary
Detailed Improvement Breakdown
Enhanced JSON Loading Efficiency
Faster Question-Answer Lookup
questions_answers_map
.Refined Error Handling for JSON Operations
Improved Code Readability with
_sanitize_text
EnhancementsAdded Application Limit Check (new feature)
Form Opening Check Enhancements
Resilient Handling for Premium Redirects
Streamlined Application Form Handling
Button Search Optimization
Optimized Job Description and Recruiter Information Retrieval
Enhanced Safety Reminder Handling (new feature)
Improved Dropdown Selection Handling
Modularized Close Modal Window Function (new feature)
Enhanced Document Generation with PDF Style Improvements
Improved Resume File Naming Convention (new feature)
Improved Error Feedback for User Notifications
Redundant Code Removal
Handling Application Limits Gracefully (new feature)
Improved Focus Reset
Sanitized Text for Consistent Answer Matching
More Resilient Form Filling with
form_sections
Handling