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

Add using directive for specifying a (default) main class #400

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

fourth44
Copy link

@fourth44 fourth44 commented Nov 18, 2021

resolves #370.
All the infrastructure for passing a main class in the build options already existed, so this only adds the directive to fill that option.

Copy link
Contributor

@lwronski lwronski left a comment

Choose a reason for hiding this comment

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

@fourth44 Thank you for your contribution! I have just left one suggestion.

): Either[BuildException, BuildOptions] = {
val mainClasses = DirectiveUtil.stringValues(values)
val options = BuildOptions(
mainClass = mainClasses.lastOption
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, we should not allowed to passing main class few times.

You can use SeveralMainClassesFoundError which is already defined here.

Copy link
Author

Choose a reason for hiding this comment

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

I'm happy to implement your suggestion, but I would like to point out that SeveralMainClassesFoundError seems to be intended for the number of main classes found by analyzing the classpath, while this situation is about the number of main classes supplied by directives. That said, supplying multiple --main-class command line arguments also results in an error so this should definitely too, and it seems best to reuse this error rather than be pedantic that it's not about found but supplied main classes.

Copy link
Contributor

@lwronski lwronski Nov 22, 2021

Choose a reason for hiding this comment

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

I think too, that is better to reuse SeveralNoClassdefFoundError in this case.

Anyway, we started working on an options validation mechanism, which should detect when a directive is specified multiple times, so for now your proposal I good to merge. Later, we will integrate a new mechanism with your directive.

@fourth44 fourth44 force-pushed the using-directives-main-class branch 3 times, most recently from 8c59aaf to 25f7e7c Compare November 19, 2021 15:34
@SethTisue
Copy link
Contributor

is there a ticket about kebab-case vs camelCase for directives? it may seem bike-shed-y, but it's the sort of decision that's hard to change later, too

@lwronski lwronski self-requested a review November 22, 2021 10:27
@lwronski lwronski merged commit 5f3248d into VirtusLab:master Nov 22, 2021
@lwronski
Copy link
Contributor

lwronski commented Nov 24, 2021

is there a ticket about kebab-case vs camelCase for directives? it may seem bike-shed-y, but it's the sort of decision that's hard to change later, too

Hi @SethTisue, Here is opened the discussion to unify standard for naming convention in directives.

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.

Add using directive for --main-class
3 participants