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

feat: add templates #59

Merged
merged 6 commits into from
Nov 2, 2022
Merged

feat: add templates #59

merged 6 commits into from
Nov 2, 2022

Conversation

shamsartem
Copy link
Collaborator

fix: fluence run --dry

fix: fluence run --dry
@linear
Copy link

linear bot commented Oct 28, 2022

DXJ-141 Add templates

Add reasonable templates for ts, js

Add default aqua input and output to fluence.yaml so there is no need to provide it

Also don't ask for output on fluence aqua --dry

https://fluencelabs.slack.com/archives/C03B529E2KY/p1666473770713229?thread_ts=1666472600.829269&cid=C03B529E2KY

@shamsartem shamsartem self-assigned this Oct 28, 2022
@shamsartem shamsartem requested a review from coder11 October 28, 2022 10:56

export const ensureFluenceJSDeployedAppPath = (fluenceJSDir: string): string =>
path.join(fluenceJSDir, DEPLOYED_APP_JS_FILE_NAME);
export const ensureFluenceJSAppPath = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these paths configurable? I mean JS and TS destination paths.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

now yes

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome

@@ -142,52 +160,52 @@ import "@fluencelabs/aqua-lib/builtin.aqua"

-- import App from "deployed.app.aqua"
-- import Adder from "services/adder.aqua"
-- export App, add_one
-- export App, addOne
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are doing snake_case to camelCase renaming only for the sake of making generated TS files look nice than I think we are doing it in the wrong place. This should be done inside aqua compiler.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DieMyst WDYT?

@folex WDYT about recommended naming notation?

Copy link
Member

Choose a reason for hiding this comment

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

I like snake case. But I don't care much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think compiler shouldn't do that. It's just camelCase is a lot more popular among js developers so I think people who compile to js would prefer using camelCase themselves

Copy link
Contributor

Choose a reason for hiding this comment

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

Our built-ins library is using snake case in function naming. So if a developer would be using camelCase they would inevitably face style inconsistencies

@shamsartem shamsartem requested a review from coder11 November 2, 2022 08:30
Copy link
Contributor

@coder11 coder11 left a comment

Choose a reason for hiding this comment

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

Feel free to merge the changes if you need them ASAP. But I would suggest to come to some sort of an agreement on naming style and fix the aqua template accordingly.

@shamsartem
Copy link
Collaborator Author

I will merge this for now. If we decide something different - I will just do another PR with a fix

@shamsartem shamsartem merged commit 7d6142d into main Nov 2, 2022
@shamsartem shamsartem deleted the DXJ-141 branch November 2, 2022 16:31
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.

3 participants