V3.2 features and deprecation #172
Conversation
Signed-off-by: Shahm Najeeb <[email protected]>
Removed the --extra flag, and gave a deprecation warning to it, will be removed later however, also added new feature to Flag.py that allows you to print the flag help menu whenever needed! Signed-off-by: Shahm Najeeb <[email protected]>
Signed-off-by: Shahm Najeeb <[email protected]>
Also updated PULL_REQUEST_TEMPLATE.md Signed-off-by: Shahm Najeeb <[email protected]>
…nto v3.2-features-and-deprecation
DefinetlyNotAI
added
request/Minor
WalkthroughThis pull request updates various files to enhance the handling of deprecated features across the project. Key changes include adding a "Deprecation Change" checkbox to the pull request template, removing outdated training scripts in VulnScan, modifying configuration files, and improving error handling in several scripts. The overall aim is to streamline the codebase and prepare for future enhancements. Changes
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🪛 LanguageToolREADME.md[typographical] ~187-~187: Consider adding two commas here.
(HOWEVER_COMMA) [style] ~190-~190: ‘future plans’ might be wordy. Consider a shorter alternative.
(EN_WORDINESS_PREMIUM_FUTURE_PLANS) [uncategorized] ~191-~191: Possible missing preposition found.
(AI_EN_LECTOR_MISSING_PREPOSITION) 🪛 Markdownlint (0.37.0)README.md188-188: null (MD028, no-blanks-blockquote) Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Shahm Najeeb <[email protected]>
|
😭😭 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
CODE/VulnScan/v3/_generate_data.py (2)
Line range hint
192-208: Yo! These magic numbers are making my head spin! 🤔Let's make the code easier to understand and maintain by moving these hardcoded values to constants at the top of the file.
+# File generation multipliers +SENSE_MULTIPLIER = 5 +SENSE_NANO_MULTIPLIER = 0.5 +SENSE_MINI_MULTIPLIER = 1 if CODE_NAME == 'Sense': - FILE_NUM = DEFAULT_FILE_NUM * 5 - MIN_FILE_SIZE = DEFAULT_MIN_FILE_SIZE * 5 - MAX_FILE_SIZE = DEFAULT_MAX_FILE_SIZE * 5 + FILE_NUM = DEFAULT_FILE_NUM * SENSE_MULTIPLIER + MIN_FILE_SIZE = DEFAULT_MIN_FILE_SIZE * SENSE_MULTIPLIER + MAX_FILE_SIZE = DEFAULT_MAX_FILE_SIZE * SENSE_MULTIPLIER elif CODE_NAME == 'SenseNano': - FILE_NUM = 5 - MIN_FILE_SIZE = int(DEFAULT_MIN_FILE_SIZE * 0.5) - MAX_FILE_SIZE = int(DEFAULT_MAX_FILE_SIZE * 0.5) + FILE_NUM = int(DEFAULT_FILE_NUM * SENSE_NANO_MULTIPLIER) + MIN_FILE_SIZE = int(DEFAULT_MIN_FILE_SIZE * SENSE_NANO_MULTIPLIER) + MAX_FILE_SIZE = int(DEFAULT_MAX_FILE_SIZE * SENSE_NANO_MULTIPLIER) elif CODE_NAME == 'SenseMini': - FILE_NUM = DEFAULT_FILE_NUM - MIN_FILE_SIZE = DEFAULT_MIN_FILE_SIZE - MAX_FILE_SIZE = DEFAULT_MAX_FILE_SIZE + FILE_NUM = int(DEFAULT_FILE_NUM * SENSE_MINI_MULTIPLIER) + MIN_FILE_SIZE = int(DEFAULT_MIN_FILE_SIZE * SENSE_MINI_MULTIPLIER) + MAX_FILE_SIZE = int(DEFAULT_MAX_FILE_SIZE * SENSE_MINI_MULTIPLIER)
Line range hint
336-342: Heads up! Let's make this logging more secure! 🔒When logging file paths and names, we should be careful not to expose sensitive info in the logs.
- log.debug(f"Zipping directory '{directory}' with name '{name}' under action '{ACTION}'") + # Avoid logging full paths that might contain sensitive information + safe_dir = os.path.basename(directory) + log.debug(f"Zipping directory '{safe_dir}' with name '{name}' under action '{ACTION}'")
🧹 Nitpick comments (7)
CODE/VulnScan/README.md (1)
3-7: The warning is helpful but could be clearer! 📢The warning about moving VulnScan is super important, but let's make it pop more:
-> [!WARNING] -> VulnScan will be moved to a different repository soon. -> This part of the repository will be archived and the new repository will be linked here. -> The `pkl` and `pth` files will stay here, linked properly for `vulnscan.py`, -> VulnScan will NOT be moved to the other repository. +> [!WARNING] +> # VulnScan Migration Notice 🚨 +> +> VulnScan is moving to a new home! Here's what you need to know: +> - This repository section will be archived +> - A link to the new repository will be added here +> - The `pkl` and `pth` files will stay here (they'll work with `vulnscan.py`) +> - VulnScan itself will NOT be moved to the other repositoryAlso, it might be helpful to add when this migration is happening! Could you add a timeline? 🗓️
🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ... part of the repository will be archived and the new repository will be linked here....(COMMA_COMPOUND_SENTENCE_2)
CODE/VulnScan/v3/_generate_data.py (1)
188-191: Hey! Nice deprecation warning, but let's make it more user-friendly! 🎯The warning message is good, but we could make it more helpful by telling users what they should use instead.
- if CODE_NAME == 'SenseMacro': - print( - "\033[91mDeprecationWarning: A call to the deprecated feature SenseMacro has been called, this has been removed due to instability issues, going with default settings (Model Sense).\033[0m") - CODE_NAME = 'Sense' + if CODE_NAME == 'SenseMacro': + print( + "\033[91mDeprecationWarning: SenseMacro has been removed due to instability issues. " + "Please use 'Sense' instead for better stability and performance. " + "Defaulting to 'Sense' settings for now.\033[0m") + CODE_NAME = 'Sense'CODE/logicytics/Flag.py (2)
171-185: Cool addition of deprecated flags! But let's make them super clear! 🚫The deprecated flags are marked well, but we could make them even more obvious to users.
# Deprecated Flags - v3.3 parser.add_argument( "--unzip-extra", action="store_true", - help="Unzip the extra directory zip File " - f"{cls.colorify('- Use on your own device only -', 'y')}.", + help="[DEPRECATED] Unzip the extra directory zip File. " + f"{cls.colorify('This flag will be removed in v3.3', 'r')}", ) parser.add_argument( "--extra", action="store_true", - help="Open's the extra directory menu to use more tools. " - f"{cls.colorify('- Still experimental -', 'y')} " - f"{cls.colorify('- MUST have used --unzip-extra flag -', 'b')}.", + help="[DEPRECATED] Open's the extra directory menu. " + f"{cls.colorify('This flag will be removed in v3.3', 'r')}", )
278-284: Nice help menu method! But let's make it more flexible! 🎨The help menu method is good, but we could make it more reusable by allowing custom formatting.
@staticmethod - def show_help_menu(): + def show_help_menu(format_output: bool = False): """ Displays the help menu for the Logicytics application. + + Args: + format_output (bool): If True, returns the help text instead of printing it """ _, parser = Flag.__available_arguments() - parser.print_help() + if format_output: + return parser.format_help() + else: + parser.print_help()CODE/Logicytics.py (3)
138-147: Let's make this deprecation warning pop! 💥The warning is good, but we could make it more helpful by suggesting alternatives.
- print("\033[91mDeprecationWarning: " - "A call to the deprecated feature `extra` has been called, " - "this feature has been removed as it doesn't follow " - "the required code standards or aim of Logicytics.\033[0m") + print("\033[91mDeprecationWarning: The `extra` feature has been removed! 🚫\n" + "Why? It didn't match our code quality standards.\n" + "What to use instead? Check out our new features with --help\033[0m")
179-186: Same here - let's make this warning more user-friendly! 🎯Similar to the extra feature warning, we should make this more helpful.
- print("\033[91mDeprecationWarning: " - "A call to the deprecated feature `unzip-extra` has been called, " - "this feature has been removed as it doesn't follow " - "the required code standards or aim of Logicytics.\033[0m") + print("\033[91mDeprecationWarning: The `unzip-extra` feature has been removed! 🚫\n" + "Why? It didn't match our code quality standards.\n" + "What to use instead? Check out our new features with --help\033[0m")
372-373: Hey! Let's track this TODO properly! 📝Instead of leaving a comment about future implementation, let's create an issue to track it.
Would you like me to create a GitHub issue to track the webhook implementation? I can help draft the issue with requirements and implementation details!
🛑 Comments failed to post (1)
CODE/VulnScan/README.md (1)
3-8: 💡 Codebase verification
Hey, we need to talk about this migration plan - it's kinda confusing! 🤔
The README says VulnScan will be moved but also says it won't be moved, which is super contradicting! Plus, there's some important stuff we found:
- The main
vulnscan.pydirectly uses two key files:
VulnScan/Model SenseMini .3n3.pthVulnScan/Vectorizer .3n3.pkl- These files are actually in the repo and other code depends on them too
- The training and tools code that creates these files is also in the VulnScan folder
So like, we need to clear up what's actually moving and what's staying. RN the README is giving mixed signals - it's like telling us to go left and right at the same time! 😅
🔗 Analysis chain
Let's verify the migration plan
Since this is a big change, we should make sure all the pieces fit together:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any direct dependencies on VulnScan rg -l "import.*VulnScan|from.*VulnScan" --type py # Check for any references to pkl and pth files rg -l "\.pkl|\.pth" --type pyLength of output: 234
Script:
#!/bin/bash # Let's check what these files are actually doing with pkl/pth files rg "\.pkl|\.pth" -C 2 CODE/vulnscan.py CODE/logicytics/FileManagement.py CODE/VulnScan/v3/_train.py CODE/VulnScan/tools/_vectorizer.py CODE/VulnScan/tools/_study_network.py # Also check if there are any pkl/pth files in the repo fd -e pkl -e pthLength of output: 3616
🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ... part of the repository will be archived and the new repository will be linked here....(COMMA_COMPOUND_SENTENCE_2)
|
@coderabbitai Where does the readme say about the migration? Which one? |
|
Tip For best results, initiate chat on the files or code changes. @DefinetlyNotAI: The mention of the migration appears in the VulnScan documentation at CODE/VulnScan/README.md. Specifically, a warning was added stating that VulnScan would be moved to a different repository, with the current one being archived. This includes details about leaving the .pkl and .pth files behind but removing the rest of VulnScan’s functionality. If you need the exact text, check the updated “Warning” section in CODE/VulnScan/README.md. |
Coderabbit suggestion Signed-off-by: Shahm Najeeb <[email protected]>
|
Code Climate has analyzed commit 729e737 and detected 0 issues on this pull request. View more on Code Climate. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
CODE/VulnScan/v3/_generate_data.py (1)
Line range hint
219-224: Watch out! We should add some error handling here! 🛡️When writing files with sensitive data, we should make sure to handle any potential errors and clean up properly if something goes wrong.
Here's a safer way to handle the file operations:
- with open(filepath, 'w', encoding='utf-8') as f: - f.write(content) + try: + with open(filepath, 'w', encoding='utf-8') as f: + f.write(content) + except IOError as e: + logger.error(f"Failed to write file {filename}: {e}") + # Clean up any partially written file + if os.path.exists(filepath): + os.remove(filepath) + raise
🧹 Nitpick comments (5)
README.md (2)
41-44: Let's clean up these CUDA instructions! 📝The CUDA installation instructions are a bit confusing. Let's fix those typos:
- "NVidea" should be "NVIDIA"
- "usage's" shouldn't have an apostrophe
- Sentence structure needs work
Here's a clearer version:
-If the device has CUDA available (NVidea GPUs), - -Otherwise, run `pip3 install torch torchvision torchaudio` to use the CPU, ofcourse this is optional for normal -usage's, +If the device has CUDA available (NVIDIA GPUs), use the command above. +Otherwise, run `pip3 install torch torchvision torchaudio` to use CPU. +Note: This is only required if you plan to use the `vulnscan` feature.
186-187: Quick grammar fix needed! ✍️The sentence structure could be clearer:
-Don't recreate the scripts/ideas below as then it's a waste of time for you, -unless the Side-note on the script says otherwise, you can however contribute to the script itself. +Don't recreate these scripts/ideas (it would be a waste of time), +unless the Side-note says otherwise. You can, however, contribute to the script itself.🧰 Tools
🪛 LanguageTool
[typographical] ~187-~187: Consider adding two commas here.
Context: ...te on the script says otherwise, you can however contribute to the script itself.[...
(HOWEVER_COMMA)
CODE/VulnScan/v3/_generate_data.py (2)
188-194: Hey! Let's use Python's built-in warnings module instead of print! 🐍The warning message is great, but using Python's warnings module would be more standard and give us some cool benefits like filtering and capturing warnings programmatically.
Here's how we can make it better:
+import warnings + - if CODE_NAME == 'SenseMacro': - print( - "\033[91mDeprecationWarning: SenseMacro has been removed due to instability issues. " - "Please use 'Sense' instead for better stability and performance. " - "Defaulting to 'Sense' settings for now.\033[0m" - ) - CODE_NAME = 'Sense' + if CODE_NAME == 'SenseMacro': + warnings.warn( + "SenseMacro has been removed due to instability issues. " + "Please use 'Sense' instead for better stability and performance. " + "Defaulting to 'Sense' settings for now.", + DeprecationWarning, + stacklevel=2 + ) + CODE_NAME = 'Sense'
Line range hint
226-228: Let's make this more Pythonic! 🐍Instead of using an else clause to prevent imports, we can use the standard
if __name__ == '__main__'pattern more effectively.Here's a cleaner way:
-else: - raise ImportError("This training script is meant to be run directly " - "and cannot be imported. Please execute it as a standalone script.") +def main(): + """Main function containing the script's primary logic""" + # Move all the main execution code here + pass + +if __name__ == '__main__': + main() +else: + raise ImportError("This script is meant to be run directly")CODE/Logicytics.py (1)
330-336: Let's make those file paths more reliable! 🛣️Using backslashes directly in paths isn't great - they can cause problems on different operating systems. Let's use
os.path.joininstead!Here's a more reliable way to handle paths:
def zip_and_log(directory: str, name: str): log.debug(f"Zipping directory '{directory}' with name '{name}' under action '{ACTION}'") zip_values = FileManagement.Zip.and_hash( - directory, + os.path.normpath(directory), # Normalize path separators name, ACTION if ACTION is not None else f"ERROR_NO_ACTION_SPECIFIED_{datetime.now().isoformat()}" ) if ACTION == "modded": - zip_and_log("..\\MODS", "MODS") + zip_and_log(os.path.join("..", "MODS"), "MODS")
🛑 Comments failed to post (1)
CODE/Logicytics.py (1)
138-144: 🛠️ Refactor suggestion
Hey! We've got some copy-paste going on here! 🎭
These deprecation warnings are super similar. Let's make a helper function to avoid repeating ourselves!
Here's how we can clean this up:
+def show_deprecation_warning(feature_name: str): + """Display a standardized deprecation warning for removed features.""" + print(f"\033[91mDeprecationWarning: The `{feature_name}` feature has been removed! 🚫\n" + "Why? It didn't match our code quality standards.\n" + "What to use instead? Check out our new features with --help\033[0m") + input("Press Enter to exit...") + exit(0) - if ACTION == "extra": - print("\033[91mDeprecationWarning: The `extra` feature has been removed! 🚫\n" - "Why? It didn't match our code quality standards.\n" - "What to use instead? Check out our new features with --help\033[0m") - input("Press Enter to exit...") - exit(0) + if ACTION == "extra": + show_deprecation_warning("extra") - if ACTION == "unzip_extra": - print("\033[91mDeprecationWarning: The `unzip_extra` feature has been removed! 🚫\n" - "Why? It didn't match our code quality standards.\n" - "What to use instead? Check out our new features with --help\033[0m") - input("Press Enter to exit...") - exit(0) + if ACTION == "unzip_extra": + show_deprecation_warning("unzip_extra")Also applies to: 176-180
✅ Actions performedComments resolved and changes approved. |
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
|
Caution An unexpected error occurred while opening a pull request: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
|
Caution An unexpected error occurred while opening a pull request: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
Pull Request Template
Prerequisites
--devflag, if required.PR Type
update
Description
This pull request includes several changes to deprecate outdated features, improve code organization, and update documentation. The most important changes include deprecating the
extraandunzip-extrafeatures, updating theVulnScandocumentation, and removing the_extra.pyfile.Deprecation of outdated features:
CODE/Logicytics.py: Deprecated theextraandunzip-extrafeatures and added warnings for their usage. [1] [2]CODE/logicytics/Flag.py: Moved the--extraand--unzip-extraflags to a deprecated section and added a method to display the help menu. [1] [2] [3]Documentation updates:
CODE/VulnScan/README.md: Added a warning about the upcoming move ofVulnScanto a different repository.Code organization:
CODE/VulnScan/v3/_generate_data.py: Reorganized imports and added a deprecation warning for theSenseMacrofeature. [1] [2] [3]Removal of deprecated files:
CODE/_extra.py: Removed the_extra.pyfile as part of the deprecation process.Motivation and Context
Removal of many deprecated functions, to reorganise Logicytics
Credit
N/A
Issues Fixed
N/A
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores