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

9693 lint flow #9726

Merged
merged 8 commits into from
Jul 18, 2024
Merged

9693 lint flow #9726

merged 8 commits into from
Jul 18, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Jul 17, 2024

closes: #9692, #9693

Description

Sets up a convention for flows modules and document in the orchestration README.

Also makes lint config to test them. The lint config is exported as a shared config that developers can use. I've included it in the same PR to make sure it works with the new convention.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

Documented the convention in the README. I expect it'll make its way to docs.agoric.com as needed.

Testing Considerations

nothing special

Upgrade Considerations

not yet deployed

Copy link

cloudflare-workers-and-pages bot commented Jul 17, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1a91e7b
Status:🚫  Build failed.

View logs

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

nice!

import { orchestrationFns } from './sendAnywhereFlows.js';
import * as flows from './sendAnywhere.flows.js';
Copy link
Member

Choose a reason for hiding this comment

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

👍

const orchFns = orchestrateAll(orchestrationFns, {
const orchFns = orchestrateAll(flows, {
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Nit: git is not catching this as a rename. Could you split this commit in 2 with minimal changes to the source during the file rename?

Copy link
Member Author

Choose a reason for hiding this comment

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

because you want to preserve the git blame on the lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

done, but I'll say it didn't seem worth the time to me. the reindenting already touches all the lines.

@turadg turadg requested review from mhofman and dckc July 18, 2024 17:47
@turadg turadg marked this pull request as ready for review July 18, 2024 17:47
@@ -35,6 +35,7 @@ import { preparePortfolioHolder } from '../exos/portfolio-holder-kit.js';
*/

/**
* @satisfies {OrchestrationFlow}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,24 @@
/**
* @file Module with linting errors, to verify the linting config detects them.
Copy link
Member

Choose a reason for hiding this comment

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

👏

@@ -3,3 +3,18 @@
Build feature-rich applications that can orchestrate assets and services across the interchain.

Usage examples can be found under [src/examples](https://github.com/Agoric/agoric-sdk/tree/master/packages/orchestration/src/examples). They are exported for integration testing with other packages.

## Orchestration flows
Copy link
Member

Choose a reason for hiding this comment

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

only skimmed this

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Jul 18, 2024
@mergify mergify bot merged commit c3009d4 into master Jul 18, 2024
78 of 79 checks passed
@mergify mergify bot deleted the 9693-lint-flow branch July 18, 2024 21:40
turadg added a commit to endojs/endo that referenced this pull request Jul 18, 2024
Refs: Agoric/agoric-sdk#9726

## Description

A new lint rule, `harden-exports`, to support
Agoric/agoric-sdk#9726

Includes an autofixer

### Security Considerations

Could enhance security

### Scaling Considerations

n/a

### Documentation Considerations

We don't yet document provided rules:
https://endojs.github.io/endo/modules/_endo_eslint_plugin.html

I think that's okay for now. If that is requested I'd file it as a
separate issue, out of scope of this one.

### Testing Considerations

I temporarily enabled this in the **recommended** config and ran
`lint-fix` on the repo. All the changes looked correct. However
`harden()` isn't always available so I don't think we should enable it
in any of Endo's shared configs.

### Compatibility Considerations

n/a

### Upgrade Considerations

n/a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document a best practice to have modules dedicated to contain only code for orchestration flows
3 participants