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.

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

README.md fixes #80

Merged
merged 1 commit into from
Feb 19, 2019
Merged

README.md fixes #80

merged 1 commit into from
Feb 19, 2019

Conversation

tonyhutter
Copy link
Contributor

  • Fix build instructions and spelling mistake

@codecov-io
Copy link

codecov-io commented Feb 14, 2019 •
edited
Loading

Codecov Report

Merging #80 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #80   +/-   ##
========================================
  Coverage    82.42%   82.42%           
========================================
  Files           10       10           
  Lines         1064     1064           
========================================
  Hits           877      877           
  Misses         187      187

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ec062b...c1e49d6. Read the comment docs.

DavidPoliakoff
DavidPoliakoff reviewed Feb 15, 2019
View reviewed changes
README.md Outdated
mkdir build
cd build
cmake -DCMAKE_INSTALL_PREFIX = <where you want the sofware> ..
cmake -DCMAKE_INSTALL_PREFIX=<where you want to install GOTCHA> ..
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey Tony, I don't think this will work, if you're in the root directory of the repo ".." is above the root. Suppose I'm in ~/src/gotcha (the repo root). If I cmake .. I'm getting ~/src. That said, I can see how the instructions as written are confusing, any ideas on a wording that makes sense?

@DavidPoliakoff
Copy link
Contributor

Thanks for the contribution, the spelling correction is an easy merge, appreciated. Any thoughts on the wording for the build instruction itself?

@tonyhutter
README.md fixes
c1e49d6 
- Fix spelling
- Remove spaces around '=' for -DCMAKE_INSTALL_PREFIX
- Change '..' to '../' to make it clear it's a directory and not ellipsis

Signed-off-by: Tony Hutter <hutter2@llnl.gov>
@tonyhutter
Copy link
Contributor Author

Oops, I mistakenly read the .. as ellipsis for the "<where you want the software>" text. I've changed it to ../ in my latest push to make it clear, and also restored the build lines.

@DavidPoliakoff
Copy link
Contributor

@tonyhutter : great, I hadn't thought about that ambiguity. Good catch, it'll take this a bit to pass CI, but it's going to, I'll merge once that happens.

Thanks for contributing!

@DavidPoliakoff DavidPoliakoff merged commit 8b153e2 into LLNL:develop Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidPoliakoff DavidPoliakoff DavidPoliakoff left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@tonyhutter @codecov-io @DavidPoliakoff