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

Make resolve-import-conflicts return a status code #218

Conversation

mernst
Copy link
Collaborator

@mernst mernst commented Sep 26, 2023

No description provided.

@benedikt-schesch benedikt-schesch changed the base branch from v2-update to v2-update-code-review-comments September 26, 2023 19:07
@benedikt-schesch
Copy link
Owner

@mernst Is it correct that the function issubsequence in src/scripts/merge_tools/resolve-conflicts.py takes as argument two lists of strings?

@mernst
Copy link
Collaborator Author

mernst commented Sep 27, 2023

@benedikt-schesch Thanks for the improvements!
issubsequence (which I just renamed to is_subsequence to be more Pythonic) takes arbitrary sequences as input. It does work for strings, but it is generic over the type of list elements.

@mernst mernst removed their assignment Sep 27, 2023
@benedikt-schesch
Copy link
Owner

@mernst If you set the type hints to string you can see that one of the function calls does call it with a list of strings, so I am not sure if the script is correct currently.

@mernst
Copy link
Collaborator Author

mernst commented Sep 27, 2023

@benedikt-schesch I think that is OK: the function works on any type of list, including a list of strings, and it's called with a list of strings. Is there a problem that I don't see?

@benedikt-schesch
Copy link
Owner

Then we're all good

@benedikt-schesch benedikt-schesch merged commit ac5fd88 into v2-update-code-review-comments Sep 27, 2023
@benedikt-schesch benedikt-schesch deleted the v2-update-resolve-import-conflicts-status-2 branch September 27, 2023 18:54
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.

2 participants