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

Rename lookout-sdk binary to lookout-tool #447

Merged
merged 4 commits into from Jan 9, 2019
Merged

Conversation

@smacker
Copy link
Contributor

smacker commented Jan 4, 2019

Fix: #412

@smacker smacker requested review from carlosms, dpordomingo and se7entyse7en Jan 4, 2019
docs/lookout-tool.md Outdated Show resolved Hide resolved
docs/lookout-tool.md Show resolved Hide resolved
docs/lookout-tool.md Show resolved Hide resolved
@smacker
Copy link
Contributor Author

smacker commented Jan 4, 2019

@dpordomingo how did you convert svg to png originally? I used ImageMagick and it looks different.

@dpordomingo
Copy link
Contributor

dpordomingo commented Jan 8, 2019

I did it the hacky way: screenshot → save as .png.
Your way to proceed is way maintainable, so I'd say yours is better.

ImageMagick

convert \
    docs/assets/lookout-sdk-seq-diagram.svg
    lookout-sdk-seq-diagram.png

But these other alternatives render better PNGs (specially CairoSVG), but we should find the better options to do it.

svgexport

$ svgexport docs/assets/lookout-sdk-seq-diagram.svg \
    docs/assets/lookout-sdk-seq-diagram.png \
    "svg{background:white;}"

Inkscape

$ inkscape -z -e docs/assets/lookout-sdk-seq-diagram.png \
    --export-background white \
    docs/assets/lookout-sdk-seq-diagram.svg

CairoSVG

$ cairosvg -f png --output-width 900 \
    --output docs/assets/lookout-sdk-seq-diagram.png \
    docs/assets/lookout-sdk-seq-diagram.svg

I think we could move this to a new issue, and apply its decision in all our projects.

@smacker
Copy link
Contributor Author

smacker commented Jan 9, 2019

@carlosms @dpordomingo according to David's comment we can keep png as it is right now.
Is everything else good? Are we good to merge?

@carlosms
Copy link
Contributor

carlosms commented Jan 9, 2019

We could improve the images in a different PR. But to be honest they look worse, the text typography or kerning makes it harder to read. Why don't we use the same hacky screenshot for this PR, this way we are not in a hurry to find a good way to render them automatically?

@smacker
Copy link
Contributor Author

smacker commented Jan 9, 2019

@carlosms updated using screenshot

Copy link
Contributor

dpordomingo left a comment

Inside of docs/analyzers-examples.md there is also a lookout-sdk mention that should be renamed with lookout-tool

docs/assets/lookout-tool-seq-diagram.md Outdated Show resolved Hide resolved
@smacker
Copy link
Contributor Author

smacker commented Jan 9, 2019

thanks @dpordomingo ! fixed

Copy link
Contributor

dpordomingo left a comment

LGTM
many many many thanks for taking care of this!!!

smacker added 4 commits Jan 4, 2019
Signed-off-by: Maxim Sukharev <max@smacker.ru>
Signed-off-by: Maxim Sukharev <max@smacker.ru>
Signed-off-by: Maxim Sukharev <max@smacker.ru>
Signed-off-by: Maxim Sukharev <max@smacker.ru>
@smacker
Copy link
Contributor Author

smacker commented Jan 9, 2019

rebased on master + squshed docs commits. Merging after CI is green.

@smacker smacker merged commit a2c2a11 into src-d:master Jan 9, 2019
5 checks passed
5 checks passed
DCO DCO
Details
codecov/project 63.5% (+0.07%) compared to ca74bf7
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lookout The analysis was performed
Details
carlosms added a commit to carlosms/lookout that referenced this pull request Jan 10, 2019
This reverts commit a2c2a11, reversing
changes made to ca74bf7.
carlosms added a commit to carlosms/lookout that referenced this pull request Jan 10, 2019
This reverts commit a2c2a11, reversing
changes made to ca74bf7.

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.