numerous bug fixes #228
Merged
numerous bug fixes #228
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix bug where if there were multiple issue URLs to insert into a file, after the first insertion all further insertions would copy a "random" line (based on *original* line numbering) and insert that copy rather than the issue URL. Now processes files in order by file name and then in reverse order by line number, thereby ensuring that any line insertions/ deletions at bottom of file don't affect line numbering above them. Closes github.com/alstr#225
test_new.diff had some issues which would cause errors if trying to actually use the diff file with 'patch' to create the files it references. 1) had some line numbers which were invalid 2) had path specifications and, unfortunately, 'patch' does not support creating absent directories The file has been cleaned up to have the create line numbers and to assume that all files are at the root. Additionally, for the few cases where the diff file referred to an edit rather than creation of a whole new file, those diff hunks were moved into a new, separate, file. There is no functional change with this commit, but it sets up future test features.
No functional change, but enables future changes will allow more of the code to be exercised by unittest.
Reads the diff file to generate a simulated filesystem, inserts the URL comments into these new files, and then parses the output to confirms that all of the issues that were expected to be created were created.
Insertion of the URL is different from simply creating/updating the issue, so we want to have this level of output to ensure that as many file updates as expected are actually occurring. Note: The unit test is currently failing!
There are issues currently where the wrong text may be inserted in place of the issue URL. (See github.com/alstr/todo-to-issue-action issues alstr#224 , alstr#225, and alstr#226, for example.) When such a situation is detected, the offending line is now NOT inserted and instead a warning message is printed out. This doesn't completely resolve those issues, but mitigates against them and other similar issues which may arise in the future.
When searching for the comment line after which the issue URL will be inserted, use a case-insensitive search for the identifier portion of the line. This matches how TodoParser identifies TODOs using a case-insensitive search. Closes github.com/alstr#224
If regex characters are part of the title (for example, parentheses), then this breaks the ability to find the comment line when trying to insert the issue URL. This commit escapes the title to fix this problem. Closes github.com/alstr#226
Wrong line numbering is causing issue URLs to not be inserted when processing this diff file
The diff file has a couple of hunks which for some unknown reason have parsing issues which result in the wrong line being detected for where the TODO is. This causes the issue URL insertion to fail. That's its own issue that should be investigated, but in order to allow the process_diff unit test to pass, the diff file is being split into two to isolate the trouble-causing hunks.
Added a test which demonstrates a current bug where if multiple TODOs with the same title appear in the diff of the same file, only one of those issues will have its URL succesfully inserted. (Specifically, the first from the top.) As such, the test is configured as an expected failure until such time as the bug can be fixed.
|
@alstr, FYI that this PR is ready for your review. Thanks. |
I initially thought it a good idea to split these up, but it led to a lot of unnnecessary code redundancy. This is a better approach.
|
Great work, thanks! 😄 |
This was referenced Nov 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses a number of bug fixes as well as adding a new test to demonstrate an existing bug (which this PR does not fix).
Relevant issues:
Issues #216, #224, and #225 all stem from the same root cause: the app doesn't properly handle a diff that introduces the same-titled TODO multiple times into the same file. This PR does NOT fix that root issue (a separate PR will do so though), but it DOES address the unintended side effect of introducing other, erroneous, lines into the source code. It will also prevent any future errors of this same class.