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

Updating information for Contributors #3874

Closed
wants to merge 673 commits into from

Conversation

DeNic0la
Copy link
Collaborator

@DeNic0la DeNic0la commented Sep 30, 2023

I updated the English part of CONTRIBUTING.md, would appreciate feedback before doing the German version

Copy link
Member

@manuelmeister manuelmeister left a comment

Choose a reason for hiding this comment

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

Very cool! This should help get things started.

Could you please check the text with a grammar checker (like DeepL write). Nouns usually don't start with a capital letter. Only the first letter of the sentence and "proper" nouns like specific things, people and places start with a capital letter (like Switzerland, Google & Manuel Meister).

Copy link
Member

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

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

Many words are started with an uppercase letter, please correct those.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@manuelmeister manuelmeister 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! I have tried to improve some aspects that were still a bit unclear to me.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@DeNic0la
Copy link
Collaborator Author

DeNic0la commented Oct 21, 2023

I added a Database and Discussion chapter.

I added Emojis

You can easily view them on my repository or download this JetBrains extension (tested) or a vs code extension. I am aware that not everyone is a fan of emojis. IMO they make the file look more friendly and inviting. I am convinced the overall effect of those emojis is positive so I added them.

In the Workflow chapter I added suggestions, did the spellcheck and made the following modifications:

  • Refactor checklist: use title, descriptions and add explanation
  • Code Formatting
    • Add print, pdf and e2e commands
    • List commands for Lint: show docker compose exec vs run and explain
    • Add pre-commit.sh: Example Git-Hook
    • Stuff is now collapsable
  • Use $(git config user.name) instead of 'your-username' to simplify the 'happy flow'

I split the file into CONTRIBUTING.md and CONTRIBUTING_DE.md and here are my reasons:

  • Spellcheck with one Language per file
  • Side-by-side comparison to ensure both files contain the same information
  • When scrolling through the file / Headings / Structure it feels like a 'complete' file
  • Possibility to translate (with deepl API?) files like the ./translations

I am aware that this is very biased and can all be summarized as 'I just like it more', that's why I am asking for approval/permission. Note that the German version is not up to date.

🏁 IMO the English version is complete for the moment (meaning ready to be merged).

🚧 If you have any topics the CONTRIBUTING.md should address but doesn't please let me know.
✔️ If you agree that the English version is ready for merge and agree (or at least accept/tolerate) my decision to split by language please let me know with an Approval-Review-Thingy

👷 If this Draft-PR gets approved I will update the German version and once finished mark this as ready for merge (the 'not-draft-thingy') which means you will get a second chance to review before merge. I therefore ask you to approve this Draft if your only critique is a (or multiple) misspelt word(s) and you generally approve the content, I will still fix my mistakes (if you point them out) and rerun some spellchecker before marking this as ready. By approving based on content I hope to add the German version sooner and work more efficiently.

@manuelmeister @carlobeltrame

CONTRIBUTING.md Outdated

We use a triangular git workflow. This means that all changes are first pushed to a contributor's fork of the repository, and then the changes are merged into the main fork via a pull request. In practice, setting up this workflow looks as follows:

1. Fork the main repository onto your GitHub account. Let's say your GitHub account is called `your-username`.
1. Fork the main repository onto your GitHub account. To use the commands your configured git `user.name` must be exactly your git user name.
Copy link
Contributor

Choose a reason for hiding this comment

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

To use the commands your configured git user.name must be exactly your git user name.
Is this a requirement by us?

Github just takes the user.name used in the commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is one of my bolder choices in this PR. Here is what I was thinking: the people who lack the technical knowledge to set up a git repo using a forked repo are the main concerns here. When this was the case for me I used Git over GUI Tools (Login to GitHub in Intellij, GitHub Desktop) and my Git username was always set to my GitHub username. I made the assumption that most people have git username = GitHub username (I call such cases 'happy flow', not sure if it's a common term).

In the happy flow, you can now execute every command as it is (for example using IntelliJ or copy-paste-execute) and there are no modifications needed.

If you use a different git username the only thing that changed compared to the previous version is the text you are replacing. You still need to replace text in the exact same places. Instead of replacing your-username (I believe that was the previous placeholder) you need to replace $(git config user.name). This is a bit more confusing than before, I would agree. I made the assumption that people who need to replace this and don't fall into the happy flow are a minority with the technical understanding to set this up without documentation.

There is no requirement to have your git username match your GitHub username, this is a requirement for the happy flow. This could probably be better written.

This might be an impactful change but I am convinced it will make the setup more efficient overall. Do you want me to clarify in the documentation or revert my changes?

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of describing the happy flow for beginners, and leave experienced git gurus to do things their way. Maybe you could just change "must" (and "need to" in the paragraph below) to "should", and it'll sound less harsh while still being strongly suggestive?

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
pre-commit.sh Show resolved Hide resolved
pre-commit.sh Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

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

Thanks, this is coming along great!

CONTRIBUTING.md Outdated

<em lang="de">Die <a href="#deutsch">deutsche Übersetzung</a> findest du weiter unten.</em>
# English :milky_way:
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this heading now, would you agree? It is clear from the content of the document that the language is English.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. This would also reduce the heading levels which would serve the readability and visual hierarchy!
Which would also need to be adjusted

@@ -0,0 +1,71 @@
# Mitarbeit
Copy link
Member

Choose a reason for hiding this comment

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

I like the split into two different files. Can you also adjust the link in the German part of the README.md, and ideally also in the contribution guide on the landing page (https://github.com/ecamp/ecamp-site/blob/main/src/content/pages/contribution.de.md?plain=1#L28)

CONTRIBUTING.md Outdated
### Labels :label:
Issues are marked with labels and some of them are not self-explanatory and are explained here:
- **Type-Labels**:
Type labels follow the `type: *` format with the options `Frontend`, `Print`, `Deployment` & `API` the architecture for those are partially documented in the [wiki](https://github.com/ecamp/ecamp3/wiki/architecture-frontend)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than describing semi-formally how we construct these labels, how about just listing the examples type: Frontend, type: API, etc.?

CONTRIBUTING.md Outdated
### [Code of conduct](https://www.ecamp3.ch/en/code-of-conduct)
## Workflow :gear:
This is a basic overview of the workflow, i.e. how we work with the code of eCamp v3. More information about how to set up a development environment on your computer is in the [wiki](https://github.com/ecamp/ecamp3/wiki/installation).
If something about the setup is unclear, or you run into an error, there are [discussions](https://github.com/ecamp/ecamp3/discussions) on GitHub for you to ask questions and ask for help. :computer:
Copy link
Member

Choose a reason for hiding this comment

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

Replace this sentence with a Discord invite link: https://discord.gg/tdwtRytV6P
We are sunsetting discussions for now and handling setup support on Discord.

- **Type-Labels**:
Type labels follow the `type: *` format with the options `Frontend`, `Print`, `Deployment` & `API` the architecture for those are partially documented in the [wiki](https://github.com/ecamp/ecamp3/wiki/architecture-frontend)
- **Needs prototype**: :bulb: If you have an idea how to solve this issue: we'd like to see it. This issue needs a prototype before actual implementation begins since the specifications are somewhat vague. A prototype can be many things, whether your prototype is a sketch, mockup, partial implementation or something else is up to you.
- **Good first issue**: :green_heart: Beginner friendly issues.
Copy link
Member

Choose a reason for hiding this comment

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

Move this up to the first position in the list?

CONTRIBUTING.md Outdated
Following it will not only enhance the quality and consistency of your contributions:sparkles: but also fast-track the review process. :rocket:


- [x] **Sync with Central Repository:** :arrows_counterclockwise: Ensure your fork is up to date with the central repository, facilitating a smooth merge.
Copy link
Member

Choose a reason for hiding this comment

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

- [x] **Spelling:** :abc: Ensure that the changes are spell checked
- [x] **Significant Changes:** :mag_right: Confirm that every modified file contributes meaningful content, steering clear of inconsequential changes like mere whitespace adjustments.
- [x] **Testing:** :test_tube: Write tests for any new features, and update existing ones if you've made changes to functionalities.
- [x] **Language & Spelling:** :book: Use English for all variable names, class names, functions, comments, etc., and ensure that all added content has been spellchecked.
Copy link
Member

Choose a reason for hiding this comment

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

Spelling is already mentioned above, remove either one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those two mean slightly different things. The first one is documentation (which I don't think needs to be English) and the other one is code, like variable names (which I do think needs to be (mostly) English).
However, I must admit that this adds no value and is just overly complicated so I will "merge" them.

CONTRIBUTING.md Outdated
including edge cases, are effectively covered.

### Recommended Test User :bust_in_silhouette:
To begin with, utilize the `test@example.com / test` user credentials.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To begin with, utilize the `test@example.com / test` user credentials.
To start out, utilize the `test@example.com / test` user credentials.

I think "To begin with" means more like "Überhaupt mal"

CONTRIBUTING.md Outdated
When reporting an issue or bug, consider referencing a specific example from 'Dev-Data'.
Since the data, including IDs, remains consistent, it allows everyone to easily replicate and understand the behavior you're highlighting.

## Discussions :speech_balloon:
Copy link
Member

Choose a reason for hiding this comment

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

Replace with Discord

pre-commit.sh Outdated Show resolved Hide resolved
@BacLuc
Copy link
Contributor

BacLuc commented Jan 2, 2024

Do we still need this pr after #4204?

@DeNic0la
Copy link
Collaborator Author

DeNic0la commented Jan 6, 2024

Do we still need this pr after #4204?

Nope sorry, I forgot

@DeNic0la DeNic0la closed this Jan 6, 2024
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

6 participants