From 582a35ce1aec792027f4d350a6a054a7c195cf54 Mon Sep 17 00:00:00 2001 From: Martin Gaievski Date: Thu, 30 Nov 2023 15:23:27 -0800 Subject: [PATCH 1/2] Adding code guidelines to DEVELOPER_GUIDE md Signed-off-by: Martin Gaievski --- DEVELOPER_GUIDE.md | 69 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md index 5ee672da5..6a1e27d19 100644 --- a/DEVELOPER_GUIDE.md +++ b/DEVELOPER_GUIDE.md @@ -311,6 +311,7 @@ run successfully on the PR. For example, if a PR on main needs to be backported `backport 2.x` to the PR and make sure the backport workflow runs on the PR along with other checks. Once this PR is merged to main, the workflow will create a backport PR to the `2.x` branch. + ## Building On Lucene Version Updates There may be a Lucene version update that can affect your workflow causing errors like `java.lang.NoClassDefFoundError: org/apache/lucene/codecs/lucene99/Lucene99Codec` or @@ -338,3 +339,71 @@ through the same build issue. 3. Head over to your OpenSearch cloned repo root directory 1. `./gradlew publisToMavenLocal` 4. Finally run `./gradlew build` from the neural search repo + +## Code Guidelines + +### Class and package names + +Class names should use `CamelCase`. + +Try to put new classes into existing packages if package name abstracts the purpose of the class. + +Example of good class file name and package utilization: + +`src/main/java/org/opensearch/neuralsearch/processor/factory/RerankProcessorFactory.java` + +following naming needs improvement, it creates unnecessary package and uses underscores case for file name + +`src/main/java/org/opensearch/neuralsearch/rerank_factory/rerank_processor_factory.java` + +### Modular code + +Try to organize code into small classes and methods with a single concise purpose. It's preferable to have multiple small +methods rather than a long single one and does everything. + +### Documentation + +Document you code. That includes purpose of new classes, every public method and code sections that have critical or non-trivial +logic (check this example https://github.com/opensearch-project/neural-search/blob/main/src/main/java/org/opensearch/neuralsearch/query/NeuralQueryBuilder.java#L238). + +When you submit a feature PR, please submit a new +[documentation issue](https://github.com/opensearch-project/documentation-website/issues/new/choose). This is a path for the documentation to be published as part of https://opensearch.org/docs/latest/ documentation site. + +Please be prepared to provide any additional guidance (like example of query request/response, details of API parameters etc.) for the team doing the documentation. + +### Code style + +For the most part, we're using common conventions for Java projects. Here are a few things to keep in mind. + +1. Use descriptive names for classes, methods, fields, and variables. +2. Avoid abbreviations unless they are widely accepted +3. Use `final` on all method arguments unless it's absolutely necessary +4. Wildcard imports are not allowed. +5. Static imports are preferred over qualified imports when using static methods +6. Prefer creating non-static methods whenever possible. Static methods should generally be avoided as they are often used as a shortcut. +7. Use functional programming style inside methods unless it's a performance critical section. +8. For parameters of lambda expression please use meaningful names instead of shorten cryptic ones. +9. Use Optional for return values if the value may not be present. This should be preferred to returning null. +10. Do not create checked exceptions, and do not throw checked exceptions from public methods whenever possible. In general, if you call a method with a checked exception, you should wrap that exception into an unchecked exception. +11. Throwing checked exceptions from private methods is acceptable. +12. Use String.format style in case your string is using parameters, prefer that to direct string concatenation +13. Prefer Lombok annotations to the manually written boilerplate code + +Some of mentioned styles are enforced by the `spotless` Gradle plugin, please check [Java Language Formatting Guidelines](##Java Language Formatting Guidelines) section for more details. + +### Tests + +Write unit and integration tests for your new functionality. + +Unit tests are preferred as they are cheap and fast, try to use them to cover all possible +combinations of parameters. Utilize mocks to mimic dependencies. + +Integration tests should be used sparingly, focusing primarily on the main (happy path) scenario or cases where extensive +mocking is impractical. Include one or two unhappy paths to confirm that correct response codes are returned to the user. +Whenever possible, favor scenarios that do not require model deployment. If model deployment is necessary, use an existing +model, as tests involving new model deployments are the most resource-intensive. + +### Outdated or irrelevant code + +Do not submit code that is not used or needed, even if it's commented. We rely on github as version control system, code +can be restored if needed. From eccf8b1f4b845dcf336f2f979971b9e37457e2e6 Mon Sep 17 00:00:00 2001 From: Martin Gaievski Date: Tue, 29 Oct 2024 15:28:54 -0700 Subject: [PATCH 2/2] Adding guidelines to the table of content Signed-off-by: Martin Gaievski --- DEVELOPER_GUIDE.md | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md index 6a1e27d19..db28bcc6d 100644 --- a/DEVELOPER_GUIDE.md +++ b/DEVELOPER_GUIDE.md @@ -17,6 +17,7 @@ - [Supported configurations](#supported-configurations) - [Submitting Changes](#submitting-changes) - [Building On Lucene Version Updates](#building-on-lucene-version-updates) + - [Code Guidelines](#code-guidelines) # Developer Guide @@ -311,7 +312,6 @@ run successfully on the PR. For example, if a PR on main needs to be backported `backport 2.x` to the PR and make sure the backport workflow runs on the PR along with other checks. Once this PR is merged to main, the workflow will create a backport PR to the `2.x` branch. - ## Building On Lucene Version Updates There may be a Lucene version update that can affect your workflow causing errors like `java.lang.NoClassDefFoundError: org/apache/lucene/codecs/lucene99/Lucene99Codec` or @@ -380,14 +380,20 @@ For the most part, we're using common conventions for Java projects. Here are a 3. Use `final` on all method arguments unless it's absolutely necessary 4. Wildcard imports are not allowed. 5. Static imports are preferred over qualified imports when using static methods -6. Prefer creating non-static methods whenever possible. Static methods should generally be avoided as they are often used as a shortcut. +6. Prefer creating non-static public methods whenever possible. Avoid static methods in general, as they can often serve as shortcuts. +Static methods are acceptable if they are private and do not access class state. 7. Use functional programming style inside methods unless it's a performance critical section. 8. For parameters of lambda expression please use meaningful names instead of shorten cryptic ones. 9. Use Optional for return values if the value may not be present. This should be preferred to returning null. 10. Do not create checked exceptions, and do not throw checked exceptions from public methods whenever possible. In general, if you call a method with a checked exception, you should wrap that exception into an unchecked exception. 11. Throwing checked exceptions from private methods is acceptable. -12. Use String.format style in case your string is using parameters, prefer that to direct string concatenation +12. Use String.format when a string includes parameters, and prefer this over direct string concatenation. Always specify a Locale with String.format; +as a rule of thumb, use Locale.ROOT. 13. Prefer Lombok annotations to the manually written boilerplate code +14. When throwing an exception, avoid including user-provided content in the exception message. For secure coding practices, +limit exception messages to the bare minimum and log additional details to the application logs, as user input could be maliciously crafted. + + Some of mentioned styles are enforced by the `spotless` Gradle plugin, please check [Java Language Formatting Guidelines](##Java Language Formatting Guidelines) section for more details. @@ -403,6 +409,9 @@ mocking is impractical. Include one or two unhappy paths to confirm that correct Whenever possible, favor scenarios that do not require model deployment. If model deployment is necessary, use an existing model, as tests involving new model deployments are the most resource-intensive. +If your changes could affect backward compatibility, please include relevant backward compatibility tests along with your +PR. For guidance on adding these tests, refer to the [Backwards Compatibility Testing](#backwards-compatibility-testing) section in this guide. + ### Outdated or irrelevant code Do not submit code that is not used or needed, even if it's commented. We rely on github as version control system, code