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

Fixed Spacing in Path Bug on Windows #41

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

TitusSmith33
Copy link

Pull Request

Contributors: Titus Smith

This PR is aimed at fixing Issue #36, which causes a stack trace on Windows 11 when running execexam if a space exists anywhere in the directory path.

This PR simply changes 1 line that is passed in main.py of execexam when the path to a failing check is passed. Symbax previously handled paths separately if there was a space anywhere in the directory. For example, if you had the following path:
user/documents/software engineering/exam_solution the program was splitting the path and trying to handle them one at a time rather than as one. This PR makes sure the failing path is quoted properly so that paths are handled as one, regardless of whether a space exists.

This PR should not affect code coverage.

This PR was created and tested on a Windows 11 OS. I have tested this new implementation with a space in my path, and with no spaces in my path, and they both pass without causing a stack trace anymore.

Below is line 288 of main.py, which is where I changed how paths are handled:

command = f'symbex {test_name}" -f "{failing_test_path}"'

@gkapfham
Copy link
Collaborator

Hello @TitusSmith33 can you please move this PR forward through the review process? We need to get a review from one student technical leader and from two students who did not work on the fix. Thanks, I appreciate your contribution and your work in moving this PR through the process.

@Chezka109 Chezka109 self-requested a review October 31, 2024 15:42
Chezka109
Chezka109 previously approved these changes Oct 31, 2024
Copy link
Collaborator

@Chezka109 Chezka109 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it out on MacOS and it's all good! I added a space in a solution repo and did not get a stack trace.

@gkapfham gkapfham added the bug Something isn't working label Oct 31, 2024
@hannahb09 hannahb09 self-requested a review October 31, 2024 18:48
hannahb09
hannahb09 previously approved these changes Oct 31, 2024
Copy link
Collaborator

@hannahb09 hannahb09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it to run on linux. I added a space and there was no error for it.

@AlishChhetri AlishChhetri self-requested a review October 31, 2024 18:52
AlishChhetri
AlishChhetri previously approved these changes Oct 31, 2024
Copy link
Collaborator

@AlishChhetri AlishChhetri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

execexam/main.py Outdated Show resolved Hide resolved
CalebKendra
CalebKendra previously approved these changes Oct 31, 2024
Copy link
Collaborator

@PCain02 PCain02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked for me on Windows 10! LGTM

@gkapfham
Copy link
Collaborator

Hello @TitusSmith33 will you need to update your pyproject.toml file?

Copy link
Collaborator

@PCain02 PCain02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TitusSmith33 it says there are still conflicts with the pyproject.toml. You may have to literally hit the button that says mark as resolved after you have fixed the conflicts. You can see the resolve conflicts button under "changes approved"

Copy link
Collaborator

@boulais01 boulais01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, it would appear you have updated and resolved the changes requested.

Copy link
Collaborator

@PCain02 PCain02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thank you for updating the pyproject.toml file. Note you can add your name to the authors if you want.

Copy link
Collaborator

@CalebKendra CalebKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants