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

Suggested edits for openjournals/joss-reviews/issues/4562 #84

Closed
wants to merge 7 commits into from
Closed

Suggested edits for openjournals/joss-reviews/issues/4562 #84

wants to merge 7 commits into from

Conversation

thodson-usgs
Copy link

No description provided.

ThomasThelen and others added 7 commits May 1, 2022 13:24
1. Minor revisions
1. Removed dependencies section. This should be included in the documentation, but I don't think it's necessary for the paper.
1. Added Acknowledgements section. Technically not required, but I suggest you at least acknowledge any funding support, for example https://github.com/chasmani/piecewise-regression/blob/master/paper/paper.md
@ThomasThelen
Copy link
Collaborator

Thanks for the edits! I'm taking notes on those bibliography changes :)

I have two remarks that could use some clarification, otherwise I have no problem merging.

This project was done unfunded, so as far as traditional acknowledgements go, we don't have the normal NSF grant or university to thank. This was also a fairly self contained group that worked on this package without external collaboration, and I felt weird patting ourselves on the back. Would it be non-standard to leave this section out?

With the dependencies section, we were hoping to give credit to the other softwares that we used, similar to how this paper did it. Is there any way we can keep it in?

@thodson-usgs
Copy link
Author

Yes, I'm fine with both requests. If JOSS has precedent for including a dependencies section, then do so.

@ThomasThelen
Copy link
Collaborator

ThomasThelen commented Jul 15, 2022

@thodson-usgs I'm not sure if I have permissions to edit your PR (I think there's a button for allowing editing access from maintainers). Can you check to see if that box is checked on your PR?

Otherwise I think we should

  • Remove the Acknowledgements section
  • Re-add the Dependencies section
  • Remove italics on _require_ (when I was using _must_ it was in the RFC definition sense)
  • Remove the trailing comma on properties ranging from simple constructs, (I think it's a comma splice with the commas that follow)
  • Change the base to merge into develop, that way the development branch isn't being main

Thank you for the PR!

@thodson-usgs
Copy link
Author

@ThomasThelen
Yes, I'm happy all with that.
Regarding:
"Remove italics on require (when I was using must it was in the RFC definition sense)"
I prefer require, but that's a reasonable reason to use must. When discussing functions, I think it's more common to discuss 'required' and 'optional' parameters.

As for merging, I think its simplest for you to make the edits manually, and we close this PR. That's lame, I know, but I'm busy. I should be able to resume the review next week.

@ThomasThelen
Copy link
Collaborator

Not lame at all! I appreciate all of the effort you've put into this review and the PR.

@ThomasThelen ThomasThelen mentioned this pull request Jul 23, 2022
@ThomasThelen
Copy link
Collaborator

Changes were made and merged in #87, to the paper branch. We appreciate the feedback and edits, thank you!

@ThomasThelen
Copy link
Collaborator

I ended up tracking the issue where I couldn't submit changes to the commit to my Ghostery plugin. I apologize for any inconveniences I caused there there

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.

None yet

2 participants