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.

Sign up for GitHub

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

Jump to bottom

Add main field to juvix.yaml #2118

Closed
wants to merge 6 commits into from
Closed

Add main field to juvix.yaml #2118

wants to merge 6 commits into from

Conversation

janmasrovira
Copy link
Collaborator

@janmasrovira janmasrovira commented May 23, 2023 •
edited
Loading

This pr adds the field main to juvix.yaml. This field is optional and should contain a path to a juvix file that is meant to be used for the compile (and dev compile) command when no file is given as an argument in the CLI. This makes it possible to simply run juvix compile if the main is specified in the jvuix.yaml.

I have updated the juvix.yaml of the milestone examples.

@janmasrovira janmasrovira added this to the 0.3.5 milestone May 23, 2023
@janmasrovira janmasrovira self-assigned this May 23, 2023
@janmasrovira janmasrovira linked an issue May 23, 2023 that may be closed by this pull request
Add :def command to REPL for Juvix term definitions #2072
Closed
@janmasrovira janmasrovira marked this pull request as ready for review May 23, 2023 16:02
@jonaprieto jonaprieto added the enhancement New feature or request label May 23, 2023
jonaprieto
jonaprieto requested changes May 23, 2023
View reviewed changes
app/App.hs
exitMsg' :: ExitCode -> Text -> Sem r x
exitMsg' exitCode t = embed (putStrLn t >> hFlush stdout >> exitWith exitCode)
getMainFile' :: Maybe (AppPath File) -> Sem r (Path Abs File)
getMainFile' m = case m of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
getMainFile' m = case m of
getMainFile' = \case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

app/App.hs
getMainFile' m = case m of
Just p -> embed (prepathToAbsFile invDir (p ^. pathPath))
Nothing -> case pkg ^. packageMain of
-- TODO do something special if it is the global package?
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean by "special"?

Copy link
Collaborator Author

@janmasrovira janmasrovira May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking that maybe we want to explicitly forbid the main field in the global project.
I don't think there is ever a usecase for running juvix compile outside of a project.
By default, the global project doesn't have a main file by default, so maybe we leave it as it is and remove the TODO comment

app/App.hs
missingMainErr =
exitMsg'
(ExitFailure 1)
( "A path to the main file must be given in the CLI or specified in the `main` field of the "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
( "A path to the main file must be given in the CLI or specified in the `main` field of the "
( "File path to the main file is not provided. Ensure to specify it in the `main` field of the "

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 can't see that this improves the error message. Moreover, it is incorrect, because it does not mention the CLI

tests/smoke/Commands/compile.smoke.yaml
Comment on lines +21 to +25
./HelloWorld
exit-status: 0
stdout: |
hello world!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add a test to see the error in the absence fo the main field in the juvix. yaml?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@jonaprieto jonaprieto force-pushed the 2072-add-inspect-command-to-repl-for-juvix-term-definitions branch from 96fd11c to a2d4c3d Compare May 23, 2023 16:44
@janmasrovira janmasrovira force-pushed the 2072-add-inspect-command-to-repl-for-juvix-term-definitions branch from a2d4c3d to 897bdc7 Compare May 23, 2023 16:56
@janmasrovira janmasrovira deleted the 2072-add-inspect-command-to-repl-for-juvix-term-definitions branch May 23, 2023 16:59
@janmasrovira
Copy link
Collaborator Author

janmasrovira commented May 23, 2023 •
edited
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonaprieto jonaprieto jonaprieto requested changes

Assignees

@janmasrovira janmasrovira

Labels
CLI enhancement New feature or request package-manager
Projects
None yet
Milestone
0.3.5
Development

Successfully merging this pull request may close these issues.

Add :def command to REPL for Juvix term definitions Enhance compile subcommand functionality for projects
2 participants
@janmasrovira @jonaprieto