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

4.1.0 #276

Merged
merged 14 commits into from
Jun 4, 2024
Merged

4.1.0 #276

merged 14 commits into from
Jun 4, 2024

Conversation

JensAstrup
Copy link
Owner

What's Changed

  • Improve the ability to determine the field name that a specific story custom field value is associated with
  • Allow for directly accessing epic owners epic.owners: Member[]

Internal

  • Various dependency upgrades
  • Upgrade custom eslint base config
  • Add MembershipProfile to documentation

dependabot bot and others added 11 commits May 14, 2024 00:58
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
if (this.customField) {
return Promise.resolve(this.customField)
}
const service: CustomFieldsService = new CustomFieldsService({headers: getHeaders()})
Copy link

Choose a reason for hiding this comment

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

🚫 [eslint] <@stylistic/object-curly-spacing> reported by reviewdog 🐶
A space is required after '{'.

Suggested change
const service: CustomFieldsService = new CustomFieldsService({headers: getHeaders()})
const service: CustomFieldsService = new CustomFieldsService({ headers: getHeaders()})

if (this.customField) {
return Promise.resolve(this.customField)
}
const service: CustomFieldsService = new CustomFieldsService({headers: getHeaders()})
Copy link

Choose a reason for hiding this comment

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

🚫 [eslint] <@stylistic/object-curly-spacing> reported by reviewdog 🐶
A space is required before '}'.

Suggested change
const service: CustomFieldsService = new CustomFieldsService({headers: getHeaders()})
const service: CustomFieldsService = new CustomFieldsService({headers: getHeaders() })

* Get the name of the custom field that this story custom field is associated with.
*/
get name(): Promise<string> {
return this.field.then((field) => field.name)
Copy link

Choose a reason for hiding this comment

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

🚫 [eslint] <@stylistic/arrow-parens> reported by reviewdog 🐶
Unexpected parentheses around single function argument having a body with no curly braces.

Suggested change
return this.field.then((field) => field.name)
return this.field.then(field => field.name)

@@ -16,6 +16,17 @@ describe('Story Custom Field', () => {
expect(customField).toBeInstanceOf(StoryCustomField)
})

it('should return saved custom field', async () => {
const storyCustomField = new StoryCustomField({fieldId: '1'} as StoryCustomFieldInterface)
Copy link

Choose a reason for hiding this comment

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

🚫 [eslint] <@stylistic/object-curly-spacing> reported by reviewdog 🐶
A space is required after '{'.

Suggested change
const storyCustomField = new StoryCustomField({fieldId: '1'} as StoryCustomFieldInterface)
const storyCustomField = new StoryCustomField({ fieldId: '1'} as StoryCustomFieldInterface)

@@ -16,6 +16,17 @@ describe('Story Custom Field', () => {
expect(customField).toBeInstanceOf(StoryCustomField)
})

it('should return saved custom field', async () => {
const storyCustomField = new StoryCustomField({fieldId: '1'} as StoryCustomFieldInterface)
Copy link

Choose a reason for hiding this comment

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

🚫 [eslint] <@stylistic/object-curly-spacing> reported by reviewdog 🐶
A space is required before '}'.

Suggested change
const storyCustomField = new StoryCustomField({fieldId: '1'} as StoryCustomFieldInterface)
const storyCustomField = new StoryCustomField({fieldId: '1' } as StoryCustomFieldInterface)

@@ -26,4 +37,18 @@ describe('Story Custom Field', () => {
expect(customField.values).toEqual([{fieldId: '1', value: 'value'}])
Copy link

Choose a reason for hiding this comment

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

🚫 [eslint] <@stylistic/object-curly-spacing> reported by reviewdog 🐶
A space is required before '}'.

Suggested change
expect(customField.values).toEqual([{fieldId: '1', value: 'value'}])
expect(customField.values).toEqual([{fieldId: '1', value: 'value' }])

canonicalName: 'fieldName',
createdAt: new Date(),
description: 'description',
values: [{fieldId: '1', value: 'value'}]
Copy link

Choose a reason for hiding this comment

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

🚫 [eslint] <@stylistic/object-curly-spacing> reported by reviewdog 🐶
A space is required after '{'.

Suggested change
values: [{fieldId: '1', value: 'value'}]
values: [{ fieldId: '1', value: 'value'}]

canonicalName: 'fieldName',
createdAt: new Date(),
description: 'description',
values: [{fieldId: '1', value: 'value'}]
Copy link

Choose a reason for hiding this comment

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

🚫 [eslint] <@stylistic/object-curly-spacing> reported by reviewdog 🐶
A space is required before '}'.

Suggested change
values: [{fieldId: '1', value: 'value'}]
values: [{fieldId: '1', value: 'value' }]

description: 'description',
values: [{fieldId: '1', value: 'value'}]
} as unknown as CustomFieldInterface))
const customStoryField: StoryCustomField = new StoryCustomField({fieldId: '1'} as StoryCustomFieldInterface)
Copy link

Choose a reason for hiding this comment

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

🚫 [eslint] <@stylistic/object-curly-spacing> reported by reviewdog 🐶
A space is required after '{'.

Suggested change
const customStoryField: StoryCustomField = new StoryCustomField({fieldId: '1'} as StoryCustomFieldInterface)
const customStoryField: StoryCustomField = new StoryCustomField({ fieldId: '1'} as StoryCustomFieldInterface)

description: 'description',
values: [{fieldId: '1', value: 'value'}]
} as unknown as CustomFieldInterface))
const customStoryField: StoryCustomField = new StoryCustomField({fieldId: '1'} as StoryCustomFieldInterface)
Copy link

Choose a reason for hiding this comment

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

🚫 [eslint] <@stylistic/object-curly-spacing> reported by reviewdog 🐶
A space is required before '}'.

Suggested change
const customStoryField: StoryCustomField = new StoryCustomField({fieldId: '1'} as StoryCustomFieldInterface)
const customStoryField: StoryCustomField = new StoryCustomField({fieldId: '1' } as StoryCustomFieldInterface)

Copy link

github-actions bot commented Jun 4, 2024

This Pull Request introduces several changes across the project, focusing on integration with ESLint, CI pipeline adjustments, version increments, and code improvements in functionality and readability. Here's a detailed review:

ESLint Configuration (.eslintrc.cjs)

  • The addition of ...baseConfig.parserOptions and ...baseConfig.plugins provides a way to extend or override options and plugins from a base configuration. This is a good practice for maintaining consistency and reusability across projects or different parts of the same project.

GitHub Actions Workflow (.github/workflows/lint.yml)

  • Changing from a generic linting step to using tj-actions/eslint-changed-files@v25 for running ESLint only on changed files is a significant improvement. It optimizes the CI process by reducing unnecessary checks on unchanged files, which can speed up the CI pipeline and save computational resources.

package.json

  • Version is updated to 4.1.1-alpha.0, indicating new features or fixes that are not yet fully released. The use of semantic versioning is appropriate here.
  • The update of eslint-config-yenz to ^5.2.1 and the inclusion of eslint-plugin-jest at ^28.5.0 suggests an improvement in the project's linting strategy, potentially adding more strict rules or adapting the project to new best practices.
  • Modifying the lint and lint:fix scripts to narrow down the scope to the src directory is a precise decision that aligns with the common practice of focusing linting efforts on source code over tests or configuration files.

Code Changes in src/epics/epic.ts

  • The import additions and adjustments improve clarity on what external dependencies the module relies on, which aids in maintainability.
  • Transforming whitespace and formatting for consistency enhances readability and adheres to coding standards.
  • Adding a new owners getter with documentation comments showcases attention to providing clear, maintainable code and enhancing functionality.

Changes in Exports (src/index.ts)

  • Including MemberProfile in exports directly affects the project's modularity and reusability. This change makes it easier to consume the MemberProfile component elsewhere in the project or by external projects.

TypeScript Configuration Changes

  • Including tests in the include array of tsconfig.base.json integrates TypeScript checking into your test files, which is a best practice for ensuring type safety across the entire codebase.

General Observations

  • The updates across multiple files show a concerted effort to improve coding standards, enforce stricter linting rules, and streamline the CI process.
  • The incremental versioning and the shift towards more descriptive linters highlight a maturity in handling project dependencies and devOps processes.

Overall, these changes positively impact the project's maintainability, code quality, and development process efficiency.

Copy link

github-actions bot commented Jun 4, 2024

Code Review Feedback

.eslintrc.cjs Changes

  • Good Practice (spread operators in JSON Objects): Using the spread operator to extend the base ESLint configuration (baseConfig.parserOptions and baseConfig.plugins) is a good way to avoid duplication and keep the configuration DRY👍.

.github/workflows/lint.yml Changes

  • Best Practice: Switching to tj-actions/eslint-changed-files@v25 with a specific token configuration for the GitHub Action to run the linter on changed files is indeed a best practice. It focuses linting efforts on new or amended code, improving efficiency in the CI process.

package.json Changes

  • Dependency Update: Updating dependencies such as eslint-config-yenz to ^5.2.1 and adding new ones like eslint-plugin-jest signifies good maintenance of the project's dependencies ensuring the project is up-to-date with the latest features and security patches.
  • Script Adjustment: Modifying lint scripts to focus on the src directory ("lint": "eslint --ext ts src" and "lint:fix": "eslint src --ext .ts --fix") streamlines the linting process by focusing on source files, which is a sensible change.

src/epics/epic.ts and Other TypeScript Files

  • Code Style Consistency: Introducing spacing within import braces and when calling functions with object arguments ({ headers: getHeaders() }) enhances readability and consistency across the codebase. Great attention to stylistic detail👍.
  • Error Handling Enhancement: Including detailed catch blocks with type annotations ((error: AxiosError)) and processing to handle response failure adds robustness to the error handling mechanism. It's a practical approach to manage failed HTTP requests and convert API fields properly.
  • New Feature Implementation: The addition of the get owners(): Promise<Member[]> method in the Epic class is a good extension of functionality, opening up new capabilities for the application.

src/index.ts Changes

  • Export Organization: Grouping and exporting interfaces and services at the end of index.ts file makes the entry point clean and organizes the exported members well, which is beneficial for maintainability.

Test Files Changes

  • Test Improvements: The modifications and additions to the test cases, especially with async methods and the awaited expects, demonstrate a commitment to maintaining a strong testing regimen as the codebase evolves. It ensures that new features and changes are verified systematically.

Configuration and Lock file Changes

  • Project Configuration Enhancements: Updates to TypeScript configuration files to include tests in the compilation context ("include": ["./src/**/*.ts", "./tests/**/*.ts"]) is a positive change, ensuring that TypeScript checks are applied across the entire project for consistency and error detection.

General Observations

  • The changes made show a good understanding of TypeScript, modern JavaScript practices, and efficient CI/CD processes.
  • Attention to detail in terms of code style, dependency management, and feature enhancement is evident across the updates.
  • Continuous efforts to improve and refactor the codebase for better readability, maintainability, and efficiency are commendable.

Copy link

github-actions bot commented Jun 4, 2024

Here are a few observations and suggestions regarding the changes in the pull requests:

  1. .eslintrc.cjs Changes:

    • You've correctly used the spread operator to merge baseConfig.parserOptions and baseConfig.plugins into the existing configuration. This is a great approach for maintaining extendability and reducing redundancy.
  2. .github/workflows/lint.yml Changes:

    • It's an excellent improvement to switch from a generic linting step to using tj-actions/eslint-changed-files@v25. This should significantly speed up the CI process by only linting the changed files.
  3. package.json Changes:

    • Upgrading to "version": "4.1.0" from "version": "4.0.1" is appropriate given the changes.
    • Changing the eslint and eslint-plugin-jest to their newer versions as part of package updates is a good maintenance practice. It helps in keeping the dependencies up to date.
    • Modifying the linting scripts to specifically target the src directory is a smart change, assuming that's where all relevant code resides.
  4. Code Styling and Consistency in JavaScript Files:

    • The inclusion of spaces inside object literals ({ headers: getHeaders() } instead of {headers: getHeaders()}) is a good practice for readability.
    • Adding AxiosError and AxiosResponse types in epic.ts improves the code's type safety and clarity.
    • The addition of the owners method in epic.ts is consistent with the existing structure of the class. Just ensure that the ownerIds are appropriately defined and used elsewhere in the application.
    • Utilizing generics in convertApiFields<ThreadedCommentApiData, ThreadedCommentInterface>(data) to explicitly type the conversion is a type-safe practice, enhancing code quality and maintainability.
  5. tsconfig Files:

    • Removing specific include fields from tsconfig.cjs.json and tsconfig.esm.json might lead to unintended behaviors during compilation, depending on your build setup. It's crucial to ensure that TypeScript correctly identifies all necessary files. Using tsconfig.base.json to define shared configurations is a good practice.
  6. Tests:

    • Refining imports in the tests (tests/epics/epic.test.ts) to use the @sx/ path alias ensures consistency throughout the codebase. It's also positive to observe the addition of tests for new functionality like the owners getter.
  7. Dependency Upgrades and Additions:

    • The upgrade of eslint-config-yenz from ^2.0.1 to ^5.2.1 and addition of eslint-plugin-jest at ^28.5.0 are notable. These changes suggest an emphasis on maintaining code quality and leveraging the latest tooling capabilities.

In conclusion, this pull request contains numerous beneficial modifications and improvements, focusing on linting efficiency, dependency management, and code quality. The changes are thoughtful and consistent with best practices in JavaScript and TypeScript development.

Copy link

github-actions bot commented Jun 4, 2024

The submitted changes for review incorporate various updates across the project's configuration files, workflow automation, and TypeScript sources. Here's an overview:

ESLint Configuration .eslintrc.cjs

  • You've nicely integrated updates by spreading baseConfig.parserOptions and baseConfig.plugins into the respective sections in the ESLint configuration. This ensures that the base configuration options and plugins are correctly incorporated, promoting consistency and reducing redundancy. Good job!

GitHub Actions Workflow .github/workflows/lint.yml

  • Optimizing the linting process by targeting only changed files with tj-actions/eslint-changed-files@v25 is an excellent improvement. It can significantly save time and computational resources during continuous integration (CI), especially in large projects.

Package Definition package.json

  • Updating the version number to 4.1.0 signifies the introduction of new features or significant changes, adhering to semantic versioning principles. Good attention to detail!
  • Modifying the linting scripts to target the src directory directly, and adjusting the script for lint fixes, aligns with best practices for managing project structure and ensuring code quality.
  • Adding eslint-plugin-jest enhances linting for Jest test files, which is great for maintaining tests' quality and consistency.

TypeScript Sources and Tests

  • Enhancing TypeScript source files with additional imports (AxiosError, AxiosResponse) and refining type usage (convertApiFields<ThreadedCommentApiData, ThreadedCommentInterface>) displays a good understanding of leveraging TypeScript's type system for better type safety and clarity.
  • Introducing a more robust handling of asynchronous operations with proper typing in methods like Epic.comment and Epic.addComment improves code reliability and maintainability.
  • The expansion in test coverage, including new tests for Epic.owners, showcases diligence in ensuring that new functionalities are correctly tested.

General Observations

  • The restructuring and additions to exports in src/index.ts are thoughtfully done, making the public API more intuitive and accessible.
  • Enhancements in class properties, such as in StoryCustomField, introduce more efficient and safer ways to manage state and relationships between entities.
  • The introduction of additional comments and documentation within the source code is commendable. It aids in maintainability and makes the codebase more approachable for other developers.

In summary, the submitted changes are comprehensive and well-considered, reflecting a thorough understanding of the technologies and best practices involved. They contribute to the project's maintainability, performance, and adherence to coding standards. Excellent work!

@JensAstrup JensAstrup merged commit 110047d into main Jun 4, 2024
16 checks passed
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.

1 participant