Skip to content

activate_deactivate_gettingstarted_Doc #685

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
kmruehl merged 4 commits into from
Aug 25, 2021
Merged

activate_deactivate_gettingstarted_Doc #685

kmruehl merged 4 commits into from
Aug 25, 2021

Conversation

@salhus
Copy link
Contributor

@salhus salhus commented Aug 25, 2021

Cleaned up PR of the previous activate deactivate PR

@salhus
activate_deactivate_gettingstarted
6415bd1 
Cleaned up PR of the previous activate deactivate PR
@salhus
Copy link
Contributor Author

salhus commented Aug 25, 2021

@jtgrasb @nathanmtom

I think when i initiated the github check, some additional files got created. This PR has 3 files changed. The two scripts and the relevant doc.

@salhus salhus mentioned this pull request Aug 25, 2021
Closed
nathanmtom
nathanmtom reviewed Aug 25, 2021
View reviewed changes
nathanmtom
nathanmtom reviewed Aug 25, 2021
View reviewed changes
nathanmtom
nathanmtom reviewed Aug 25, 2021
View reviewed changes
@salhus
Copy link
Contributor Author

salhus commented Aug 25, 2021

Hi @nathanmtom
Thanks so much for pointing that out.
I moved the Note where the scripts are first described.
I also added the necessary ^^^^^

nathanmtom
nathanmtom approved these changes Aug 25, 2021
View reviewed changes
Copy link

@nathanmtom nathanmtom left a comment

Choose a reason for hiding this comment

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

@salhus Thanks for making the changes to the documentation. All good on my end here to pull into the dev branch. Let's ask for any final comments from the development team and then merge this PR on Friday.

@salhus
Copy link
Contributor Author

salhus commented Aug 25, 2021

Cheers @nathanmtom !

@kmruehl kmruehl added the Feature new feature request label Aug 25, 2021
@kmruehl
Copy link
Collaborator

kmruehl commented Aug 25, 2021

@nathanmtom and @salhus I'll take a look right now

@kmruehl kmruehl self-requested a review August 25, 2021 19:53
@kmruehl
Copy link
Collaborator

kmruehl commented Aug 25, 2021

@salhus I made some revisions to this PR, including:

  • renamed 'work' directory to 'temp'
  • minor updates to activate_wecsim so that a warning is not produced
  • deactivate_wecsim now removes 'temp' directory
  • updated documentation for adding wec-sim to path, see comment below

General comment:
I agree with @jtgrasb's comment #638 (review) regarding the workflow to add WEC-Sim to the MATLAB path. This PR is a great option for installing and uninstalling WEC-Sim, but it requires installation of WEC-Sim every time MATLAB is opened, whereas modifying startup.m installs WEC-Sim automatically. There is a use case for both options, so we should support both options. I've updated the docs to reflect two options for Step 1.

kmruehl
kmruehl approved these changes Aug 25, 2021
View reviewed changes
Copy link
Collaborator

@kmruehl kmruehl left a comment

Choose a reason for hiding this comment

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

This PR is now ready for a merge

@kmruehl kmruehl merged commit c2bd0ba into WEC-Sim:dev Aug 25, 2021
@salhus
Copy link
Contributor Author

salhus commented Aug 25, 2021

@kmruehl Thanks for reviewing it ! I like your edits. Thanks.

@kmruehl
Copy link
Collaborator

kmruehl commented Aug 25, 2021

@salhus no problem, thanks for adding this feature! You can delete your branch if you like.

@salhus salhus deleted the activate_deactivate_aug25 branch August 25, 2021 21:37
@kmruehl kmruehl mentioned this pull request Aug 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kmruehl kmruehl kmruehl approved these changes

+1 more reviewer

@nathanmtom nathanmtom nathanmtom approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature new feature request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@salhus @kmruehl @nathanmtom