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

Remove the broken badge, add coding guidelines and simplify PR/issue templates #86

Merged
merged 6 commits into from
Dec 5, 2024

Conversation

vedran-kasalica
Copy link
Member

@vedran-kasalica vedran-kasalica commented Sep 25, 2024

Two GitHub actions (build and type checking) were combined into one, so only one badge is needed.

The templates for the PR and Issues were simplified.

Contributing.md was updated to include coding guidelines (python code, docstrings, testing, etc.).

Two GitHub actions (build and type checking)  were combined into one, so only one badge is needed.
@vedran-kasalica vedran-kasalica changed the title Remove the broken badge from README.md Remove the broken badge and simplify PR/issue templates Sep 26, 2024
@vedran-kasalica vedran-kasalica changed the title Remove the broken badge and simplify PR/issue templates Remove the broken badge, add coding guidelines and simplify PR/issue templates Sep 26, 2024
@vedran-kasalica
Copy link
Member Author

I am done making changes, the PR can be reviewed. They reflect some of our discussions today (offline within the rest of the team) about the coding standards and guidelines.

Comment on lines 1 to 8
---
name: Bug report
about: Create a report to help us improve
title: ''
labels: bug
assignees: ''

---
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason you suggest to have this table at the top of the issue/PR? It confuses me a little, in particular because the "about" is also clarified at "bug details", "name" is similar to "labels", and "assignees" is usually chosen manually. What am I missing here?


## Type of change

Please delete options that are not relevant.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Please delete options that are not relevant.
Please select relevant options (can be multiple).

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you suggest working with checkboxes here, perhaps simply (un)checking is sufficient.

## Checklist before requesting a review

- [ ] I have read the [contribution guidelines](https://github.com/biomarkersParkinson/tsdf/blob/main/CONTRIBUTING.md)
- [ ] I have commented my code, particularly in hard-to-understand areas
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have documented my code, particularly in hard-to-understand areas

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, but perhaps this is more appropriate phrasing.


## Related Issue(s)
<!-- List related issues or tasks that this pull request addresses. Use the format `#issue_number` to automatically link them here. -->
Please delete options that are not relevant.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar changes as in prior suggestion if agreed upon.

Copy link
Contributor

@Erikpostt Erikpostt left a comment

Choose a reason for hiding this comment

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

Looking good Vedran, thank you for adding these changes to the tsdf repo. I have several comments that you can find above. Let me know what you think.

Copy link
Contributor

@Erikpostt Erikpostt left a comment

Choose a reason for hiding this comment

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

Adjusted several of the templates, ready for merging.

@Erikpostt Erikpostt merged commit cf5fab3 into main Dec 5, 2024
1 check passed
@Erikpostt Erikpostt deleted the remove-badge branch December 5, 2024 09:39
@vedran-kasalica
Copy link
Member Author

Hi @Erikpostt somehow I didn't get the notifications for your comments (maybe because I was not tagged), otherwise I would have replied of course. I got one now that it was merged. Thanks for resolving it!

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.

2 participants