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

Make ConfigOptions.access an enum #66

Closed
Tracked by #101
liamnichols opened this issue Jul 30, 2022 · 2 comments · Fixed by #118
Closed
Tracked by #101

Make ConfigOptions.access an enum #66

liamnichols opened this issue Jul 30, 2022 · 2 comments · Fixed by #118
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@liamnichols
Copy link
Member

/// Access level modifier for all generated declarations
public var access: String = "public"

The access property is currently just a string meaning that you can put anything in the config file and we'll end up generating non-functioning code.

CreateAPI will only support public or internal access levels since open can't be applied everywhere and private would just generate code that is unusable. We could model this with an enum instead.

One thing to note is that while we could specify access: internal in the config, we'd want to generate code without access level specifiers (i.e the same as an empty string) since the internal keyword itself in generated code is redundant.

For reference, we've used enums elsewhere before in ConfigOptions so the same pattern can be followed:

/// Available levels of indentation
public enum Indentation: String, Codable {
case spaces
case tabs
}
/// Change the style of indentation. Supported values:
/// - `spaces`
/// - `tabs`
public var indentation: ConfigOptions.Indentation = .spaces

@liamnichols liamnichols added enhancement New feature or request good first issue Good for newcomers labels Jul 30, 2022
@LePips
Copy link
Contributor

LePips commented Jul 31, 2022

Couldn't we just assume one as default and leave the other as a flag? I would assume public, since people most likely will have these as a separate package and then internal can be under isInternalAccess.

@liamnichols
Copy link
Member Author

Yeah that's also a good idea.

It would require deprecating the old access flag though so might be a little tricker than I thought initially as there are a few options to potentially deprecate in the future, we might want a slightly nicer way to manage them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants