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

[REVIEW]: General binary file parser. #766

Closed
whedon opened this Issue Jun 6, 2018 · 33 comments

Comments

Projects
None yet
6 participants
@whedon
Collaborator

whedon commented Jun 6, 2018

Submitting author: @jfjlaros (J.F.J. Laros)
Repository: https://github.com/jfjlaros/bin-parser
Version: 0.0.20
Editor: @danielskatz
Reviewer: @hainesr, @Smattr
Archive: 10.5281/zenodo.1295625

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/345778df6874891e84c712cc0d665549"><img src="http://joss.theoj.org/papers/345778df6874891e84c712cc0d665549/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/345778df6874891e84c712cc0d665549/status.svg)](http://joss.theoj.org/papers/345778df6874891e84c712cc0d665549)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@hainesr & @Smattr, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @danielskatz know.

Review checklist for @hainesr

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (0.0.20)?
  • Authorship: Has the submitting author (@jfjlaros) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @Smattr

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (0.0.20)?
  • Authorship: Has the submitting author (@jfjlaros) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@whedon

This comment has been minimized.

Collaborator

whedon commented Jun 6, 2018

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @hainesr, it looks like you're currently assigned as the reviewer for this paper 🎉.

⭐️ Important ⭐️

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands
@whedon

This comment has been minimized.

Collaborator

whedon commented Jun 6, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Collaborator

whedon commented Jun 6, 2018

@danielskatz

This comment has been minimized.

Collaborator

danielskatz commented Jun 6, 2018

@hainesr & @Smattr - over to you now to start your reviews, following the instructions above.

Please let me know if you have any questions or run into any problems.

@hainesr

This comment has been minimized.

Collaborator

hainesr commented Jun 7, 2018

Hi @danielskatz, for the version check, does there need to be an actual tagged release and corresponding GitHub release (i.e. here: https://github.com/jfjlaros/bin-parser/releases)?

@danielskatz

This comment has been minimized.

Collaborator

danielskatz commented Jun 7, 2018

@hainesr - I think so. @arfon, can you confirm?

@danielskatz

This comment has been minimized.

Collaborator

danielskatz commented Jun 8, 2018

👋 @arfon - see question above

@Smattr

This comment has been minimized.

Collaborator

Smattr commented Jun 9, 2018

@jfjlaros very nice project! The included presentation also gives a very nice motivation for how this project came about. I've posted a series of issues resulting from my review thus far. Some high level questions for you and perhaps @arfon as well:

  1. Would you like us to submit nitpick feedback on coding style/conventions as well? E.g. more Pythonic idioms for doing certain things. If so, should these come as a separate issue or a PR or something else?
  2. There seems to be very little error handling. I triggered multiple exceptions with malformed data or structure descriptions. This seems fine to me as this tool is a reversing aid not meant for end users, but perhaps JOSS has guidelines on this? From a personal standpoint, I think reversers generally prefer a bare exception trace as it helps you better debug your spec and incrementally reverse a format.
@jfjlaros

This comment has been minimized.

jfjlaros commented Jun 9, 2018

Dear @Smattr,

Thank you for your kind words and your thorough review. I will try to address the issues you raised, and perhaps implement some of the features you requested. The latter will depend mainly on implementation time, but I think some can be addressed rather quickly.

As for detailed feedback, it is certainly welcome. I try to follow the recommended coding styles of the two programming languages used, but in this particular case I also try to keep the two code bases as similar as possible for maintainability reasons. This may result in some awkward code here and there. In any case, I would appreciate both general comments about my coding style and detailed remarks in any form you see fit.

I expected remarks about error handling and like you I am still on the fence on whether to implement it.

@danielskatz

This comment has been minimized.

Collaborator

danielskatz commented Jun 9, 2018

@Smattr

  1. Nitpicking items (that don't affect your checking boxes in the review, but would be nice to have) should probably be issues or PRs in the repo.
  2. Re error handling - if you think this should block acceptance (ie you think the software really needs to have this to be useful), then let us know that - otherwise, again an issue in the repo that informs the developer but doesn't affect this submission would be good
@Smattr

This comment has been minimized.

Collaborator

Smattr commented Jun 9, 2018

I think it's fine to leave as-is with regards to the error handling. I'll submit a PR with my nitpicks and we can discuss in more detail on that. Thanks, both.

@arfon

This comment has been minimized.

Member

arfon commented Jun 10, 2018

Hi @danielskatz, for the version check, does there need to be an actual tagged release and corresponding GitHub release (i.e. here: https://github.com/jfjlaros/bin-parser/releases)?

@hainesr - I think so. @arfon, can you confirm?

Yes please.

@hainesr

This comment has been minimized.

Collaborator

hainesr commented Jun 10, 2018

Thanks @arfon. I have raised an issue.

@hainesr

This comment has been minimized.

Collaborator

hainesr commented Jun 11, 2018

@danielskatz - I think I am happy now, with the proviso that jfjlaros/bin-parser#18 is fixed once the other suggestions have been made.

@jfjlaros - This is a really nice tool. I don't have a use-case for something like this right now, but it would have made my life easier numerous times in the past.

@jfjlaros

This comment has been minimized.

jfjlaros commented Jun 11, 2018

Dear @hainesr,

Thank you very much for your suggestions and fixes.

@hainesr

This comment has been minimized.

Collaborator

hainesr commented Jun 11, 2018

@danielskatz, @arfon - There is a question about whether the version pointed to by this submission can change now there have been lots of updates to this project here: jfjlaros/bin-parser#18.

@danielskatz

This comment has been minimized.

Collaborator

danielskatz commented Jun 11, 2018

yes, it's fine to change the version, in both the paper.md and the repo

@Smattr

This comment has been minimized.

Collaborator

Smattr commented Jun 15, 2018

At this point, I'm happy with the submission after the Python 3 compatibility branch is merged (jfjlaros/bin-parser#8) and we have a tagged version (jfjlaros/bin-parser#18). Well done addressing everything so fast, @jfjlaros.

@jfjlaros

This comment has been minimized.

jfjlaros commented Jun 15, 2018

I released v1.0.0.

Thanks @Smattr, @hainesr for your suggestions and contributions. It really improved the quality of the package.

@jfjlaros

This comment has been minimized.

jfjlaros commented Jun 16, 2018

I added credits in c1f2e6e, do we also want these in the release?

@danielskatz

This comment has been minimized.

Collaborator

danielskatz commented Jun 21, 2018

@jfjlaros , I think that's up to you.

Let me know if you think this is ready to publish, or if you want to make any final changes.

@jfjlaros

This comment has been minimized.

jfjlaros commented Jun 21, 2018

@danielskatz, I added the credits and automatic testing with Travis to v1.0.1. I think that wraps it up.

@danielskatz

This comment has been minimized.

Collaborator

danielskatz commented Jun 21, 2018

Is 1.0.1 archived somewhere with a DOI? If not, please do this.

And then paste the DOI here.

@jfjlaros

This comment has been minimized.

jfjlaros commented Jun 21, 2018

I archived it at Zenodo: 10.5281/zenodo.1295625.

@danielskatz

This comment has been minimized.

Collaborator

danielskatz commented Jun 21, 2018

@whedon commands

@whedon

This comment has been minimized.

Collaborator

whedon commented Jun 21, 2018

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Open the review issue
@whedon start review

🚧 🚧 🚧 Experimental Whedon features 🚧 🚧 🚧

# Compile the paper
@whedon generate pdf

@danielskatz

This comment has been minimized.

Collaborator

danielskatz commented Jun 21, 2018

@whedon set 10.5281/zenodo.1295625 as archive

@whedon

This comment has been minimized.

Collaborator

whedon commented Jun 21, 2018

OK. 10.5281/zenodo.1295625 is the archive.

@danielskatz

This comment has been minimized.

Collaborator

danielskatz commented Jun 21, 2018

👋 @arfon - over to you for final acceptance

@Smattr, @hainesr - Thanks for your reviewing and contributions!

@arfon arfon added the accepted label Jun 21, 2018

@arfon

This comment has been minimized.

Member

arfon commented Jun 21, 2018

@hainesr, @Smattr - many thanks for your reviews here and to @danielskatz for editing this submission

@jfjlaros - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00766 ⚡️ 🚀 💥

@arfon arfon closed this Jun 21, 2018

@whedon

This comment has been minimized.

Collaborator

whedon commented Jun 21, 2018

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippet:

[![DOI](http://joss.theoj.org/papers/10.21105/joss.00766/status.svg)](https://doi.org/10.21105/joss.00766)

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

@jfjlaros

This comment has been minimized.

jfjlaros commented Jun 21, 2018

@arfon, @danielskatz: Thank you for editing this submission.

@Smattr, @hainesr: Thank you for your suggestions and contributions. I really appreciate it.

@Smattr

This comment has been minimized.

Collaborator

Smattr commented Jun 22, 2018

This was the first JOSS review I've been involved with and I thought the whole thing went very well. Thanks all :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment