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

refactor: maintainability improvements #114

Merged
merged 26 commits into from
Jun 29, 2024

Conversation

carlocorradini
Copy link
Contributor

@carlocorradini carlocorradini commented Jun 11, 2024

This PR makes a few changes to the repository to make it easier to manage and/or utilize generally:

Changes

  • Added Dependabot
    Automatically monitor and fix vulnerable dependencies
  • Added CODEOWNERS
    File to define individuals or teams that are responsible for code in a repository
  • Better CI/CD (see .github/workflows)
  • Added clippy
    A collection of lints to catch common mistakes and improve your Rust code.
    See lints in Cargo.toml
    Some code has been refactored (no breaking changes) to fix all lints
  • Added .vscode directory
  • Added EditorConfig
    maintain consistent coding styles for multiple developers working on the same project across various editors and IDEs
  • Added .gitattributes
    Defining attributes per path
  • Better .gitignore
  • Cargo.lock should be ignored in libraries (added in .gitignore)
  • Fixed some typos
  • Better README.md (all *.md files in general) with better badges.

Let me know what you think 🤗🥳🤯

@obmarg
Copy link
Owner

obmarg commented Jun 11, 2024

Thanks @carlocorradini but you really should have checked before going to the effort of putting this together.

I don't have the time to reply to each individual change just now, but most of these changes are not things I want.

@carlocorradini
Copy link
Contributor Author

@obmarg
Don't worry 🤗
It didn't take me that much 🙌
Tell me what you don't like/want and I'll refactor the PR 😊👌

Copy link
Owner

@obmarg obmarg left a comment

Choose a reason for hiding this comment

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

Sorry for the delay getting back to you @carlocorradini - been a busy week at work.

  • Added Dependabot
    Automatically monitor and fix vulnerable dependencies

I'm planning to use renovate for this, see #90. I just need to find the time to finish configuring it as I want.

  • Added CODEOWNERS
    File to define individuals or teams that are responsible for code in a repository

You can keep this, but I'm not sure what the purpose is. It's my repo, obviously I am the owner.

  • Better CI/CD (see .github/workflows)

In what way is this better? I may be missing something, but it looks like you just rename the files? If there are improvements I'm missing let me know, but I'd rather not rename the files without a good reason, so if you can revert that please.

  • Added clippy
    A collection of lints to catch common mistakes and improve your Rust code.
    See lints in Cargo.toml
    Some code has been refactored (no breaking changes) to fix all lints

I already use clippy on this repo, but I'm generally happy with its default settings. I'll comment in Cargo.toml with more details though.

I'd rather not dictate a users vscode settings - can you remove this please?

_Workspace settings_
  • Added EditorConfig
    maintain consistent coding styles for multiple developers working on the same project across various editors and IDEs

I won't be using this myself so I'm not sure that there's any benefit to others using it? It could easily not match the settings I've got. I'm not that bothered about most of the settings in there either tbh - rustfmt deals with the majority of the formatting in the repo.

Is there anything particularly useful in this? It's got more entries than there are files in this repository - and the vast majority of entries in this file are not likely to every be present in this repo.

  • Cargo.lock should be ignored in libraries (added in .gitignore)

I disagree, can you put the Cargo.lock back please.

  • Fixed some typos

Cool, thanks.

  • Better README.md (all *.md files in general) with better badges.

I don't think the formatting changes are an improvement, can you revert those. I was also kind of fine with the badges as they were, what is better about these?

Cargo.toml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
tests/subscription_server/mod.rs Outdated Show resolved Hide resolved
src/ws_stream_wasm.rs Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.github/workflows/rust.yaml Outdated Show resolved Hide resolved
.github/workflows/rust.yaml Outdated Show resolved Hide resolved
@carlocorradini
Copy link
Contributor Author

Sorry for the late reply; I'll fix them as soon as possible.

@carlocorradini
Copy link
Contributor Author

.gitattributes file is a common one that I use in all my projects and comes from the gitattributes repository.
The file is very useful to have a common/shared knowledge of line terminator to avoid possible commit changes across different environments (Widnows, Apple, Linux, ...)

@carlocorradini
Copy link
Contributor Author

Cargo.lock should NOT be committed.
See official issue.
Moreover, when you create a new library project the .gitignore file already contains Cargo.lock with a comment that it should be ignored if this is a library whereas maintained if this is a binary.

@carlocorradini
Copy link
Contributor Author

For all markdown files I've fixed some blanks that have been left and checked the formatting/styling with markdownlint (i.e. added a blank between a title and text). Moreover, fixed NewIssue in CONTRIBUTING.md

@obmarg
Copy link
Owner

obmarg commented Jun 18, 2024

Cargo.lock should NOT be committed. See official issue. Moreover, when you create a new library project the .gitignore file already contains Cargo.lock with a comment that it should be ignored if this is a library whereas maintained if this is a binary.

First off - it doesn't really matter what a cargo issue from 2014 says. It's my project and I want to commit Cargo.lock.

Secondly, this has not been the official stance for coming up to a year.

Third, that is not the behaviour for a new library project:

CleanShot 2024-06-18 at 11 37 19@2x

@carlocorradini
Copy link
Contributor Author

@obmarg Awesome 🥳
I didn't know about that blog post 🤗

@carlocorradini
Copy link
Contributor Author

@obmarg Everything should be fixed now 🤗🥳

.github/workflows/rust.yaml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
.gitattributes Outdated Show resolved Hide resolved
@carlocorradini
Copy link
Contributor Author

@obmarg Done 😊

.github/workflows/rust.yml Outdated Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
@carlocorradini
Copy link
Contributor Author

Done 👍

@obmarg obmarg merged commit 1233eb5 into obmarg:main Jun 29, 2024
2 checks passed
@carlocorradini carlocorradini deleted the maintainability branch June 29, 2024 18:03
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