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 configuration option fail_on_warnings #23

Merged
merged 6 commits into from
Feb 22, 2019

Conversation

Jeehut
Copy link
Contributor

@Jeehut Jeehut commented Feb 21, 2019

This is a naive fix for #22. I tested this by installing my fork via Mint and it worked. I'm not sure if this is complete though, as I didn't write any tests or check if this also works as command line argument directly (I just tested via configuration file).

Copy link
Contributor

@ileitch ileitch left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! Mostly good, just a few wording changes.

Also please do test the command line argument, and add an entry to the CHANGELOG.

Sources/PeripheryKit/Configuration.swift Outdated Show resolved Hide resolved
Sources/PeripheryKit/Frontend/Commands/ScanCommand.swift Outdated Show resolved Hide resolved
Sources/PeripheryKit/Frontend/Commands/ScanCommand.swift Outdated Show resolved Hide resolved
@ileitch
Copy link
Contributor

ileitch commented Feb 22, 2019

Awesome thanks, could you add a changelog entry too?

@Jeehut
Copy link
Contributor Author

Jeehut commented Feb 22, 2019

I'm testing the command line argument at the moment and will add a CHANGELOG entry if no issues arise. Stay tuned ...

@Jeehut
Copy link
Contributor Author

Jeehut commented Feb 22, 2019

Do the command line arguments matter if there's a configuration file already? Cause it didn't work in my project when running periphery scan --strict with this configuration file:

project: NewProjectTemplate.xcodeproj
schemes: [App]
targets: [App, Tests, UITests]
report_exclude:
  - App/Generated/SwiftGen/*.swift
  - App/SupportingFiles/*.swift
retain_public: false
quiet: true

Running periphery scan with this configuration works though:

project: NewProjectTemplate.xcodeproj
schemes: [App]
targets: [App, Tests, UITests]
report_exclude:
  - App/Generated/SwiftGen/*.swift
  - App/SupportingFiles/*.swift
retain_public: false
strict: true
quiet: true

@ileitch
Copy link
Contributor

ileitch commented Feb 22, 2019

Ah yes, you need to populate the configuration for command-line options. You'll need to do this in both ScanCommand and ScanSyntaxCommand, e.g:

https://github.com/peripheryapp/periphery/blob/master/Sources/PeripheryKit/Frontend/Commands/ScanCommand.swift#L48

@Jeehut
Copy link
Contributor Author

Jeehut commented Feb 22, 2019

Makes sense, I added it as suggested to both. Testing now ... will report once done.

Please note that I already added a CHANGELOG entry and used the conventions from the SwiftGen project (see their CHANGELOG) and the SwiftLint project (theirs) here, too since there's none yet. Feel free to change the structure if you want to use a different style, I just thought since this open sourced very recently you might not have thought about one yet.

@Jeehut
Copy link
Contributor Author

Jeehut commented Feb 22, 2019

Yes, it works both with periphery scan --strict and periphery scan-syntax --strict ..
Should be ready to merge! 🎉

@ileitch
Copy link
Contributor

ileitch commented Feb 22, 2019

CHANGELOG format looks good 👍

@ileitch ileitch merged commit 928853e into peripheryapp:master Feb 22, 2019
@Jeehut
Copy link
Contributor Author

Jeehut commented Feb 22, 2019

@ileitch Thanks for reviewing and merging so quickly. 👍

Unfortunately the CHANGELOG doesn't seem to render like in the linked documents, where everything is on one line, but here it uses mutiple lines ... dunno why, but can be fixed over time, shouldn't be a huge problem. 😅

@ileitch
Copy link
Contributor

ileitch commented Feb 22, 2019

Yep no worries, thanks for the submission!

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.

2 participants