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

Documentation: Update README (12.11.2024) #115

Merged
merged 20 commits into from
Nov 15, 2024

Conversation

Gero1999
Copy link
Collaborator

@Gero1999 Gero1999 commented Nov 12, 2024

Issue

Closes #110

Description

Current README in the main branch is outdated, does not contain all required information and even has some incorrect statements. Should me updated to the current state of the package.

Definition of Done

Update all relevant content

How to test

  • Installation guidelines work well
  • Content information is updated and accurate

Feel free to modify or add content as well!

Contributor checklist

  • Code passes lintr checks
  • Code passes all unit tests
  • New logic covered by unit tests
  • New logic is documented

@Gero1999 Gero1999 linked an issue Nov 12, 2024 that may be closed by this pull request
@js3110 js3110 self-requested a review November 14, 2024 12:15
Copy link
Collaborator

@js3110 js3110 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@m-kolomanski m-kolomanski left a comment

Choose a reason for hiding this comment

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

Hi, from my side:

  • I do not think it is currently useful to install the package directly from github. We cannot run the application like that since the dependencies will not load, we will need to fix that. So after installation, you still have to change the directory to the installation directory, which makes more sense when the repo is cloned directly - I have left only this part in, as I think it makes the most sense
  • I have updated the badges, as the links pointed towards shiny repository, not aNCA
  • I have updated links to contributing guidelines
  • I have added a *Quick start - so the basics of app usage and what the user can do to test it out upon first startup. Please read and verify that it is correct, maybe some other workflow makes more sense - then please change.

In addition, I have some comments and requested changes. The contents seem to be copied over from a template, please make sure only applicable parts are included and that the links point in the right directions.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@m-kolomanski m-kolomanski requested a review from Gotfrid November 15, 2024 07:23
@Gero1999
Copy link
Collaborator Author

Hi, from my side:

  • I do not think it is currently useful to install the package directly from github. We cannot run the application like that since the dependencies will not load, we will need to fix that. So after installation, you still have to change the directory to the installation directory, which makes more sense when the repo is cloned directly - I have left only this part in, as I think it makes the most sense
  • I have updated the badges, as the links pointed towards shiny repository, not aNCA
  • I have updated links to contributing guidelines
  • I have added a *Quick start - so the basics of app usage and what the user can do to test it out upon first startup. Please read and verify that it is correct, maybe some other workflow makes more sense - then please change.

In addition, I have some comments and requested changes. The contents seem to be copied over from a template, please make sure only applicable parts are included and that the links point in the right directions.

Hello Mateusz! You are right, our main branch has commented code that I was not sure if to keep it perhaps for the future, but I just deleted it and solve the issues you mentioned. I will test with some others non-programming based people to check if they manage to understand completely the instructions on their own, as I know that sometimes this can be a game changer for our users.

And many thanks for making the changes, I think now it looks much better! 😎

Copy link
Collaborator

@Gotfrid Gotfrid left a comment

Choose a reason for hiding this comment

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

README looks good to me, but regarding the installation - I believe that we should strive to providing ability to run the app via package installation. I tried to quickly check it, and I uncovered a few issues that I could address and make it happen.

@m-kolomanski so in the current state I do agree with you: it's hard to run the app via installing the package, but I suggest that I create a separate issue and work on bringing that ability to the project.

@m-kolomanski m-kolomanski self-requested a review November 15, 2024 10:43
Copy link
Collaborator

@m-kolomanski m-kolomanski left a comment

Choose a reason for hiding this comment

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

I agree with @Gotfrid , changing how the package loads dependencies is out of scope of this issue and that is why I have suggested removing the information altogether. Lets create a separate issue as @Gotfrid suggests, and then update README alongside when that option is available.

@Gero1999 Gero1999 merged commit 622603c into main Nov 15, 2024
8 of 9 checks passed
@m-kolomanski m-kolomanski deleted the documentation/update-readme-121124 branch January 23, 2025 08:00
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.

Documentation: update README
4 participants