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

Updates in the README #2750

Merged
merged 7 commits into from
Aug 28, 2024
Merged

Updates in the README #2750

merged 7 commits into from
Aug 28, 2024

Conversation

rjlanari
Copy link
Contributor

@rjlanari rjlanari commented Aug 21, 2024

Tasks

  • PR name contains story or task reference
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@rjlanari rjlanari requested review from goapunk and m4ra August 21, 2024 08:25
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

Thank you Ricardo! Few extra things are needed to make it a bit more specific.

README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -13,17 +13,18 @@ adhocracy+ is designed to make online participation easy and accessible to every

### Requirements

* nodejs (+ npm)
* nodejs (+ npm) (Use Homebrew to reinstall the node in the projects folder runnign: brew install node)
Copy link
Contributor

Choose a reason for hiding this comment

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

here good to note that for MacOS it needs homebrew for installing node.
make install will take care to install the npm packages for the project, so no need to write down the reinstallation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took it out! thanks!

README.md Outdated
@@ -40,7 +41,7 @@ adhocracy+ is designed to make online participation easy and accessible to every

### Use postgresql database for testing

run the following command once:
To use postgresql database in production run the following command once:
Copy link
Contributor

Choose a reason for hiding this comment

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

the header for this section indicates is for using postgresql for testing, so should say as the original. some developers prefer to use postgresq instead of sqlite3, me included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I leave that out!

README.md Outdated
- the celery config parameter "always eager" is disabled (add `CELERY_TASK_ALWAYS_EAGER = False` to your `local.py`)
To find local.py run: find . -name "local.py"
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no local.py when installing from the repo. We need to create it under adhocracy_plus/config/settings.
Nice to add that the local.py can also have other customised settings such as a postgresql database setting for development.

Also would be good to point here that the celery always eager is disabled so that tests can run the celery tasks from the code that is calling them rather than scheduling them with the redis broker.

And we could end this celery section by pointing to our extensive celery documentation.
on line 79 below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make the changes as you point out!

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks. once you make and push your changes, click the refresh button next to my reviewer name

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a typo now schedulingmac

Copy link
Contributor

Choose a reason for hiding this comment

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

remove the line To find local.py run: find . -name "local.py".
Instead add: local.py sould be under adhocracy_plus/config/settings, create one if it doesn't exist. This file saves settings for local development.

@rjlanari rjlanari requested a review from m4ra August 21, 2024 14:05
README.md Outdated
* python 3.x (+ venv + pip)
* libpq (only if postgres should be used)
* sqlite3 [with JSON1 enabled](https://code.djangoproject.com/wiki/JSON1Extension)
* redis (in production, not needed for development)
* pillow-heif (required for macos M1 ...etc)
Copy link
Contributor

Choose a reason for hiding this comment

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

By ...etc I meant what you had there already ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

@rjlanari see my latest comment above.

README.md Outdated
- the celery config parameter "always eager" is disabled (add `CELERY_TASK_ALWAYS_EAGER = False` to your `local.py`)
To find local.py run: find . -name "local.py"
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a typo now schedulingmac

README.md Outdated
- the celery config parameter "always eager" is disabled (add `CELERY_TASK_ALWAYS_EAGER = False` to your `local.py`)
To find local.py run: find . -name "local.py"
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the line To find local.py run: find . -name "local.py".
Instead add: local.py sould be under adhocracy_plus/config/settings, create one if it doesn't exist. This file saves settings for local development.

@goapunk
Copy link
Contributor

goapunk commented Aug 22, 2024

more a general suggestion but maybe it would be nice to move the postgresql and celery part into a separate section (idk, something like advanced or optional) to make clear that they are not required to develop/test a+. We could also split the setup up into a mac and a linux section to make it easier to find the relevant requirement for your platform. These are just ideas though :)

@rjlanari
Copy link
Contributor Author

Thanks @m4ra for the corrections. I will put them in the file!
@goapunk, I thought of moving the postgresql and celery parts in a new section at the bottom of the file under a "Advance settings".
We can think of splitting the installations guide in macOS and Linux but I would need to know exactly what is needed for linux as I don't have it.
Let me know what you think!

@rjlanari rjlanari requested a review from m4ra August 22, 2024 10:52
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

@rjlanari I have left comments that have not been addressed yet. thank you

README.md Outdated
* python 3.x (+ venv + pip)
* libpq (only if postgres should be used)
* sqlite3 [with JSON1 enabled](https://code.djangoproject.com/wiki/JSON1Extension)
* redis (in production, not needed for development)
* pillow-heif (required for macos M1 ...etc)
Copy link
Contributor

Choose a reason for hiding this comment

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

@rjlanari see my latest comment above.

@m4ra
Copy link
Contributor

m4ra commented Aug 26, 2024

@rjlanari

@goapunk, I thought of moving the postgresql and celery parts in a new section at the bottom of the file under a "Advance settings".

I agree with the above. However splitting the installation in Linux and MacOS not necessary I find.

@rjlanari rjlanari requested a review from m4ra August 27, 2024 07:25
@@ -57,7 +73,10 @@ Go to http://localhost:8004/ and login with [email protected] | password

Copy link
Contributor

Choose a reason for hiding this comment

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

line 70 needs to go in the section Start a local server under line 40.

README.md Outdated
- the celery config parameter "always eager" is disabled (add `CELERY_TASK_ALWAYS_EAGER = False` to your `local.py`)
(Celery's always_eager is disabled to ensure tests run tasks when calling code instead of scheduling them via the Redis broker.)
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm somehow doesn't read right the when calling code, I had suggested something different, but to be more brief, rephrase it as following:
Celery's always_eager is disabled to ensure tests run the celery tasks inline instead of scheduling them via the Redis broker.

@@ -0,0 +1,3 @@
### Changes in the README

I added to the readme some notes and new comands that helped me with the installation of the a+ repository locally.
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't use personal pronouns in the changelog, it needs to be neutral writing.
we are following this format : https://keepachangelog.com/en/1.1.0/
Titles can be:

Added for new features.
Changed for changes in existing functionality.
Deprecated for soon-to-be removed features.
Removed for now removed features.
Fixed for any bug fixes.
Security in case of vulnerabilities.

So the changelog would be more like the following:

### Changed 

-  Update README with some notes and new comands for the installation of the a+ repository locally. 

README.md Outdated
We care about security. So, if you find any issues concerning security, please send us an email at info [at] liqd [dot] net.


## Advance settings
Copy link
Contributor

Choose a reason for hiding this comment

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

@rjlanari a typo here. should be Advanced settings

@rjlanari rjlanari requested a review from m4ra August 27, 2024 13:54
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

few more things!

@@ -13,17 +13,18 @@ adhocracy+ is designed to make online participation easy and accessible to every

### Requirements

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are revisiting the README, we can also add the tech docs.
On line 3 please add a link to our technical documentation.
E.g The project's technical documentation is in progress. You are welcome to leave your feedback on the documentation by creating a github issue.

README.md Outdated
@@ -37,6 +38,22 @@ adhocracy+ is designed to make online participation easy and accessible to every
### Start a local server

make watch
Go to http://localhost:8004/ and login with [email protected] | password
Copy link
Contributor

Choose a reason for hiding this comment

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

above line should be outside the code block, see original:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm this is still inside the code block @rjlanari

@@ -0,0 +1,3 @@
### Changed

- Update README with some notes and new comands for the installation of the a+ repository locally.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: commands

@rjlanari rjlanari requested a review from m4ra August 27, 2024 15:16
README.md Outdated
@@ -1,5 +1,6 @@
# adhocracy+

The project's [technical documentation] (https://liqd.github.io/adhocracy-plus/) currently is in progress. You are welcome to provide feedback by creating a GitHub issue..
Copy link
Contributor

@m4ra m4ra Aug 28, 2024

Choose a reason for hiding this comment

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

@rjlanari has to go just after the introduction of what is aplus. When I wrote on line 3 I meant that can continue the line. sorry that was not clear enough

Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

two last things :)

README.md Outdated
@@ -37,6 +38,22 @@ adhocracy+ is designed to make online participation easy and accessible to every
### Start a local server

make watch
Go to http://localhost:8004/ and login with [email protected] | password
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm this is still inside the code block @rjlanari

Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

one typo

README.md Outdated
@@ -1,7 +1,6 @@
# adhocracy+

The project's [technical documentation] (https://liqd.github.io/adhocracy-plus/) currently is in progress. You are welcome to provide feedback by creating a GitHub issue..
[adhocracy.plus](https://adhocracy.plus/) is a free Open-Source participation platform maintained and primarily developed by Liquid Democracy e.V.. It is based on [adhocracy 4](https://github.com/liqd/adhocracy4) and [Django](https://github.com/django/django).
[adhocracy.plus](https://adhocracy.plus/) is a free Open-Source participation platform maintained and primarily developed by Liquid Democracy e.V.. It is based on [adhocracy 4](https://github.com/liqd/adhocracy4) and [Django](https://github.com/django/django). The project's [technical documentation] (https://liqd.github.io/adhocracy-plus/) currently is in progress. You are welcome to provide feedback by creating a GitHub issue.
Copy link
Contributor

Choose a reason for hiding this comment

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

@rjlanari there is an extra space between[ technical documentation] and the relevant link, so the markdown isn't rendering
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! I noticed it when I saw the document! I corrected and also corrected the command blocks!

@rjlanari rjlanari requested a review from m4ra August 28, 2024 10:03
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

thank you!

@m4ra m4ra merged commit 3a67248 into main Aug 28, 2024
3 checks passed
@m4ra m4ra deleted the rl-2024-08-README-updates branch August 28, 2024 10:32
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.

3 participants