Skip to content

Fix #493: Ignore leading/trailing spaces in url #494

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 23, 2024
Merged

Fix #493: Ignore leading/trailing spaces in url #494

merged 5 commits into from
Feb 23, 2024

Conversation

Jessie-JY2728
Copy link
Contributor

Resolves #493.

@adazem009
Copy link
Contributor

Thanks for your contribution. There seems to be an issue with these changes. According to unit tests, this URL isn't parsed correctly: /scratch.mit.edu/projects/886166351
isProjectUrl() should return true, but returns false.

@adazem009
Copy link
Contributor

I think it would be much better to use std::isspace() to check for whitespace characters. For example check out these functions: https://github.com/musescore/MuseScore/blob/402fe051416bf4bc9d640ce0578603af3b07aefd/src/framework/global/stringutils.cpp#L65-L77

@Jessie-JY2728
Copy link
Contributor Author

Jessie-JY2728 commented Feb 22, 2024 •
edited
Loading

Thank you for you suggestion. Could you explain why the url should be valid? I guess it means the http is not required, is it? And it failed because the first part is empty. Also, could you give instructions how to run unit tests locally?

@adazem009
Copy link
Contributor

Thank you for you suggestion. Could you explain why the url should be valid? I guess it means the http is not required, is it? And it failed because the first part is empty. Also, could you give instructions how to run unit tests locally?

Yes, https:// isn't needed and it should be completely trimmed even if there's only /. To run unit tests locally, you can run ctest -j$(nproc --all) from the build directory. Or maybe your IDE has an option for it.

You can also add a test for this issue if you want. It's in the test/network/projecturl_test.cpp file.

@Jessie-JY2728
Copy link
Contributor Author

It is so nice working with you :) I have also configured github action on my forked repo, and my 3rd commit has passed the unit test over there

@adazem009
Copy link
Contributor

It is so nice working with you :) I have also configured github action on my forked repo, and my 3rd commit has passed the unit test over there

Actually, unit tests are supposed to run in the pull request. There's no need to create a GitHub action in the forked repository. The problem is that I have to approve the checks manually before they run in the pull request. Maybe I should disable that...

By the way, do you also want to add a test for these changes or should I do it after merging the PR?

@Jessie-JY2728
Copy link
Contributor Author

It is so nice working with you :) I have also configured github action on my forked repo, and my 3rd commit has passed the unit test over there

Actually, unit tests are supposed to run in the pull request. There's no need to create a GitHub action in the forked repository. The problem is that I have to approve the checks manually before they run in the pull request. Maybe I should disable that...

By the way, do you also want to add a test for these changes or should I do it after merging the PR?

Was in a bit hurry earlier today. Added tests with trailing and/or leading spaces. Looks fine so far.

@adazem009
Copy link
Contributor

adazem009 commented Feb 23, 2024 •
edited
Loading

Just a small detail. The newline at line 64 in projecturl.cpp shouldn't be there and was probably added by accident. Besides that everything looks good.

@Jessie-JY2728
remove newline?
36517f0 
ok you win
@Jessie-JY2728
Copy link
Contributor Author

Just a small detail. The newline at line 68 in projecturl.cpp shouldn't be there and was probably added by accident. Besides that everything looks good.

Error: There's no line 68 in that file. Do you mean: 64?
Removed. The newline at 63 was already there.

@adazem009
Copy link
Contributor

Just a small detail. The newline at line 68 in projecturl.cpp shouldn't be there and was probably added by accident. Besides that everything looks good.

Error: There's no line 68 in that file. Do you mean: 64? Removed. The newline at 63 was already there.

Yes, I meant 64, that was probably a typo.

adazem009
adazem009 approved these changes Feb 23, 2024
View reviewed changes
src/internal/projecturl.cpp Outdated
@@ -17,6 +17,19 @@ ProjectUrl::ProjectUrl(const std::string &url)
std::istringstream stream(url);
std::string str;

// Whitespaces to be removed
const char* whitespace = " \t\v\r\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better if this constant were static so that it isn't recreated whenever ProjectUrl is constructed.

src/internal/projecturl.cpp Outdated
@@ -47,3 +65,4 @@ const std::string &ProjectUrl::projectId() const
{
return m_projectId;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This newline was probably added by accident.

@adazem009 adazem009 merged commit 2e3d3df into scratchcpp:master Feb 23, 2024
@Jessie-JY2728 Jessie-JY2728 deleted the fix/ignore_white_space branch February 23, 2024 14:55
adazem009 pushed a commit that referenced this pull request Feb 24, 2024
@Jessie-JY2728 @adazem009
Fix #493: Ignore leading/trailing spaces in url (#494)
5bfc4f3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adazem009 adazem009 adazem009 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spaces before and after project URL should be ignored
2 participants
@Jessie-JY2728 @adazem009