-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
Provide diff with all changes in step-by-step example. #262
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea. My suggestion would be to move this code part at the bottom and explain all individual steps one-by-one above (not in this PR).
No idea about the code split. Seems like we should do this.
Related to #260 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we changes these to std::vector
now?
Co-authored-by: Frédéric Simonis <[email protected]>
Co-authored-by: Frédéric Simonis <[email protected]>
Co-authored-by: Frédéric Simonis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice, and very helpful, thank you!
I have not cross-checked with the latest state of the API, but the changes make sense compared to what I remember from the last time I checked.
@@ -16,6 +16,91 @@ Please add breaking changes here when merged to the `develop` branch. | |||
|
|||
## preCICE API | |||
|
|||
<!-- Split code block. See https://github.com/precice/precice.github.io/commit/74e377cece4a221e00b5c56b1db3942ec70a6272. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MakisH Do you have more details on this point? If it is simple: Let's do it now. If it is more complicated: I guess it's not critical and we can just merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand what you mean. Could you please elaborate what the problem is? I vaguely remember some issue with code highlighting in diff. Normal diff should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just prepared this for my thesis and thought it might also be useful for the documentation. It's unfortunately quite lenghty, but there are also many changes. I'm open for suggestions!
Todo
Split code block. See 74e377c. @uekerman , @fsimonis any guidelines?Does not look critical to me. We can also fix this later, if necessary.