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

Apidsl test #49

Merged
merged 1 commit into from
Aug 24, 2016
Merged

Apidsl test #49

merged 1 commit into from
Aug 24, 2016

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Aug 19, 2016

This change is Reviewable

@iphydf iphydf assigned GrayHatter and unassigned GrayHatter Aug 19, 2016
@nurupo
Copy link
Member

nurupo commented Aug 21, 2016

Reviewed 3 of 3 files at r2.
Review status: 3 of 38 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


CMakeLists.txt, line 206 [r2] (raw file):

################################################################################

add_test(format_test ${CMAKE_SOURCE_DIR}/other/astyle/format-source)

You could pass ${CMAKE_SOURCE_DIR} to the shell script as an argument in add_test() and avoid the bashism (I assume you have changed it from #!/bin/sh to #!/bin/bash just for ${BASH_SOURCE[0]}).


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Aug 23, 2016

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.


CMakeLists.txt, line 206 [r2] (raw file):

Previously, nurupo wrote…

You could pass ${CMAKE_SOURCE_DIR} to the shell script as an argument in add_test() and avoid the bashism (I assume you have changed it from #!/bin/sh to #!/bin/bash just for ${BASH_SOURCE[0]}).

Done.

Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Aug 23, 2016

Reviewed 38 of 38 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Aug 23, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


CMakeLists.txt, line 206 [r2] (raw file):

Previously, iphydf wrote…

Done.

Huh, the new commit didn't show up as a new revision in the Reviewable, instead it showed up as r3, which I reviewwd just before you wrote Done and pushed in the commit.

Will check the new commit later.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Aug 23, 2016

Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions.


other/astyle/format-source, line 5 [r4] (raw file):

set -e

SOURCE_DIR=$1

The lack of quotation marks around $1, $SOURCE_DIR in cd $SOURCE_DIR and ${CMAKE_SOURCE_DIR} in CMakeLists.txt disturbs me. Will this script do what it is supposed to do if ${CMAKE_SOURCE_DIR} contains spaces?

During last year's GSoC my student couldn't make the CMake project open in QtCreator properly, which apparently was caused by the project path having spaces in it. Took us a day to figure this out, and we managed to do that only by chance, because the student has included the command output textbox of Qt Creator in a screenshot, which allowed me to notice the spaces and ask to move the project somewhere with no spaces.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Aug 23, 2016

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Aug 24, 2016

Review status: 1 of 3 files reviewed at latest revision, 2 unresolved discussions.


other/astyle/format-source, line 5 [r4] (raw file):

Previously, nurupo wrote…

The lack of quotation marks around $1, $SOURCE_DIR in cd $SOURCE_DIR and ${CMAKE_SOURCE_DIR} in CMakeLists.txt disturbs me. Will this script do what it is supposed to do if ${CMAKE_SOURCE_DIR} contains spaces?

During last year's GSoC my student couldn't make the CMake project open in QtCreator properly, which apparently was caused by the project path having spaces in it. Took us a day to figure this out, and we managed to do that only by chance, because the student has included the command output textbox of Qt Creator in a screenshot, which allowed me to notice the spaces and ask to move the project somewhere with no spaces.

Done.

Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Aug 24, 2016

:lgtm:


Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf merged commit 7075c36 into TokTok:master Aug 24, 2016
@iphydf iphydf deleted the apidsl-test branch August 24, 2016 20:22
@iphydf iphydf modified the milestone: v0.0.1 Nov 6, 2016
This pull request was closed.
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.

3 participants