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

chore(ci): add contributing instructions #2082

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

soonum
Copy link
Contributor

@soonum soonum commented Feb 18, 2025

This change is Reviewable

@soonum soonum self-assigned this Feb 18, 2025
@cla-bot cla-bot bot added the cla-signed label Feb 18, 2025
CONTRIBUTING.md Outdated
>* direct changes to CI related files are not allowed for external contributors
>* run `make pcc` to fix any build errors before pushing commits

# Data versioning
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could be ## 8 Data versioning ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

CONTRIBUTING.md Outdated
}
```

1. Navigate to the definition of the dispatch enum of this type. This is the type inside the `#[versionize(MyTypeVersions)]` macro attribute. In general, this type has the same name as the base type with a `Versions' suffix. You should find something like
Copy link
Contributor

Choose a reason for hiding this comment

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

I made a mistake with a backtick:

Suggested change
1. Navigate to the definition of the dispatch enum of this type. This is the type inside the `#[versionize(MyTypeVersions)]` macro attribute. In general, this type has the same name as the base type with a `Versions' suffix. You should find something like
1. Navigate to the definition of the dispatch enum of this type. This is the type inside the `#[versionize(MyTypeVersions)]` macro attribute. In general, this type has the same name as the base type with a `Versions` suffix. You should find something like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

CONTRIBUTING.md Outdated
- changing the type of a field in a struct or a variant in an enum.

On the contrary, these changes are *not* data breaking:
- Renaming a type (though if it implements the `Named' trait, it can be),
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
- Renaming a type (though if it implements the `Named' trait, it can be),
- Renaming a type (though if it implements the `Named` trait, it can be),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

CONTRIBUTING.md Outdated
@@ -0,0 +1,217 @@
# Contributing to tfhe-rs

There are two ways to contribute to tfhe-rs:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be TFHE-rs ? (same for the other occurrences)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

@soonum soonum left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @IceTDrinker and @nsarlin-zama)

CONTRIBUTING.md Outdated
@@ -0,0 +1,217 @@
# Contributing to tfhe-rs

There are two ways to contribute to tfhe-rs:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

CONTRIBUTING.md Outdated
>* direct changes to CI related files are not allowed for external contributors
>* run `make pcc` to fix any build errors before pushing commits

# Data versioning
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

CONTRIBUTING.md Outdated
- changing the type of a field in a struct or a variant in an enum.

On the contrary, these changes are *not* data breaking:
- Renaming a type (though if it implements the `Named' trait, it can be),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

CONTRIBUTING.md Outdated
}
```

1. Navigate to the definition of the dispatch enum of this type. This is the type inside the `#[versionize(MyTypeVersions)]` macro attribute. In general, this type has the same name as the base type with a `Versions' suffix. You should find something like
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@soonum soonum requested a review from nsarlin-zama February 19, 2025 09:45
Copy link
Member

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

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

Looks generally good

Reviewable status: 0 of 1 files reviewed, 9 unresolved discussions (waiting on @nsarlin-zama)


CONTRIBUTING.md line 3 at r2 (raw file):

# Contributing to TFHE-rs

There are two ways to contribute to TFHE-rs:

TFHE-rs branding when not in titles is (seen in the README.md format we have) same elsewhere
**TFHE-rs**


CONTRIBUTING.md line 10 at r2 (raw file):

## 1. Setting up the project

First, you need to [fork](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/fork-a-repo) the TFHE-rs repository and follow the installation steps described in the repository [README.md](https://github.com/zama-ai/tfhe-rs/blob/main/README.md)

we don't seem to really have installation steps ? it's the getting started section ?

Also here I would use a relative path ?


CONTRIBUTING.md line 59 at r2 (raw file):

This command ensure that all the targets in the library are building correctly.
Alternatively, you might want to run a faster version of this command using:
 

is that a leftover space ?


CONTRIBUTING.md line 83 at r2 (raw file):

 * locate where the code has changed
 * add (or modify) a Cargo test filter to the corresponding `make` target in Makefile
 * run the target

maybe indicate that make test_something prints the test command in the terminal and that they can copy/paste and modify it ?


CONTRIBUTING.md line 91 at r2 (raw file):

## 4. Committing

Tfhe-rs follows conventional commit specification to have a consistent commit naming scheme and you are expected to follow it as well.

wrong capitlization of TFHE-rs


CONTRIBUTING.md line 116 at r2 (raw file):

sequenceDiagram
    autonumber
    

leftover spaces ?


CONTRIBUTING.md line 121 at r2 (raw file):

    participant Reviewer
    participant CI-pipeline
    

leftover spaces ?


CONTRIBUTING.md line 123 at r2 (raw file):

    
    Contributor ->> GitHub: Open pull-request
    GitHub -->> Contributor: Ask for CLA signing (once) 

letfover spaces


CONTRIBUTING.md line 130 at r2 (raw file):

        Contributor ->> GitHub: Make changes
    end
    Reviewer ->> GitHub: Pull-request approval    

leftover spaces

@soonum soonum requested a review from IceTDrinker February 20, 2025 07:53
@soonum soonum force-pushed the dt/chore/contributing branch 2 times, most recently from 6f9a6c2 to 2b1db6f Compare February 20, 2025 08:02
@soonum soonum force-pushed the dt/chore/contributing branch from 2b1db6f to 96afc32 Compare February 20, 2025 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants