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

New parser: file resources #734

Merged
merged 44 commits into from Apr 12, 2021
Merged

Conversation

mgray88
Copy link
Contributor

@mgray88 mgray88 commented Jun 25, 2020

Add a new parser which allows accessing any other specified files that need to be accessed from the bundle, usually with Bundle.main.url(forResource:withExtension:) and the like.

  • I've started my branch from the develop branch (gitflow)
    • And I've selected develop as the "base" branch for this Pull Request I'm about to create
  • I've added an entry in the CHANGELOG.md file to explain my changes and credit myself
    (or added #trivial to the PR title if I think it doesn't warrant a mention in the CHANGELOG)
    • Add the entry in the appropriate section (Bug Fix / New Feature / Breaking Change / Internal Change)
    • Add a period and 2 spaces at the end of your short entry description
    • Add links to your GitHub profile to credit yourself and to the related PR and issue after your description

Fixes #665, fixes #637, fixes #496, fixes #284, fixes #146, fixes #109.

@mgray88 mgray88 changed the title Feature/file resources Feature/file resources (#665) Jun 25, 2020
@djbe djbe added this to the 6.3.0 milestone Jun 25, 2020
Copy link
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

Hello @mgray88 and thanks for the draft PR!

I know it's only a draft for now, but:

Thx! 🙂

Sources/SwiftGenKit/Parsers/Resources/ResourceParser.swift Outdated Show resolved Hide resolved
Sources/SwiftGenKit/Stencil/ResourcesContext.swift Outdated Show resolved Hide resolved
@djbe djbe changed the title Feature/file resources (#665) New parser: file resources Jul 6, 2020
@djbe djbe mentioned this pull request Jul 6, 2020
@mgray88
Copy link
Contributor Author

mgray88 commented Jul 31, 2020

I'm running into the issue again where swiftlint is ignoring the //swiftlint:disable all at the top of my Generated Fixtures files. The linter output shows it linting the other generated files and successfully ignoring them. However it reaches the files I've added, and spits out file_header violations

@AliSoftware
Copy link
Collaborator

AliSoftware commented Jul 31, 2020

I can take a look tomorrow (I'm on my phone rn) to check if it's indeed what I think it is, but it could be expected. Because we have a somewhat special setup for linting our template files

Basically the way our linting script is set up on SwiftGen for the generated files in fixtures is that it intentionally removes the // swiftlint:disable all from the generated files before linting them. The goal of that is so that:

  1. We, as contributors and creators of templates that are gonna ship with SwiftGen, want to ensure we ship templates that respect some linter rules we set for ourselves. Like we want the templates that we ship with SwiftGen to be consistent in their indentation, use of spaces around colons, etc, and pass the swiftlint.yml rules of the SwiftGen repo itself
  2. But, for people using those templates in their projects, they might be using a different set of SwiftLint rules, etc, so even if they wouldn't trigger our rules they could still trigger the user's rules. Which is why all our templates include // swiftlint:disable:all once shipped and used by end users

I can get how this could be confusing and seem convoluted 😅 (we should definitively document that special setup in our CONTRIBUTING.md btw…). But that's the way we found to both disable swiftlint for end users of our templates while still be able to lint those templates for ourselves when working on SwiftGen — to ensure they are at least consistent with our style rules.

[EDIT] If you're curious, here is where the magic happens

Sources/SwiftGenKit/Parsers/Resources/ResourceParser.swift Outdated Show resolved Hide resolved
templates/resources/swift5.stencil Outdated Show resolved Hide resolved
templates/resources/swift5.stencil Outdated Show resolved Hide resolved
- Add `path` and `mimeType` to `File` type
- Recursively parse files
- Start writing tests for parser
@mgray88
Copy link
Contributor Author

mgray88 commented Jul 31, 2020

Gotcha. That all makes sense. I did notice the Scripts/SwiftLint.sh but it doesn't appear to discriminate based on folder/file names and just globs everything under Tests/Fixtures/Generated

@AliSoftware
Copy link
Collaborator

Yes it does force-lint (= remove disable:all before linting) everything under Tests/Fixtures/Generated but that's expected as that's where the fixtures generated by bundled templates are expected to be and that's what we want to lint.

Did you use the "Generate Outputs" Xcode scheme to auto-generate those fixtures from the templates (so that they can then be used by our unit tests)? Or did you write them by hand?

@djbe djbe modified the milestones: 6.3.0, 6.4.0 Aug 2, 2020
@SwiftGen-Eve
Copy link

SwiftGen-Eve commented Aug 3, 2020

1 Warning
⚠️ Big PR

Hey 👋 I'm Eve, the friendly bot watching over SwiftGen 🤖

Thanks a lot for your contribution!


Seems like everything is in order 👍 You did a good job here! 🤝

Generated by 🚫 Danger

@mgray88
Copy link
Contributor Author

mgray88 commented Aug 29, 2020

Can anyone provide insight on why the lint task is failing? All it says is Danger has failed this build but all the lint step succeeded with 0 violations. Oh wait, Danger is failing because of the CHANGELOG isn't it?

Also there is no output for the failing test. The tests build and run fine on my computer so I'm not sure how to fix them on CI It initially had a failed run, but now it doesn't?

@mgray88
Copy link
Contributor Author

mgray88 commented Aug 29, 2020

@AliSoftware @djbe Will the cli additions need to be included with this PR or a new one? I'm also working on writing up some of the documentation

@djbe
Copy link
Member

djbe commented Oct 18, 2020

I've implemented the filter changes I mentioned in one of my previous review comments, it was really needed (I think) because the old code would just go through all the files/children of a folder without applying the filter anymore. Does this matter for basic extension regexes? No. But it might've mattered for more complex cases 🤷

As discussed I've applied the other changes as well. The end result is that this parser is kind of like a mix of the Fonts and Strings parsers.

Future PRs could add extra accessors/methods depending on the file mime-type, a bit like the XCAssets templates right now.

@mgray88
Copy link
Contributor Author

mgray88 commented Oct 18, 2020

@djbe I'm glad you were able to get these changes in, work's been hectic on my end. I think it definitely looks much better. I did notice a few things in the new swift4 docs that still referenced swift5, or other files they were probably copied from

Co-authored-by: Michael Gray <mgray88@gmail.com>
@4brunu
Copy link

4brunu commented Dec 17, 2020

Any news on this? Looking forward to it 🙂

mgray88 and others added 2 commits December 17, 2020 09:24
# Conflicts:
#	.swiftpm/xcode/xcshareddata/xcschemes/swiftgen.xcscheme
#	Sources/SwiftGenCLI/templates/files/flat-swift4.stencil
#	Sources/SwiftGenCLI/templates/files/flat-swift5.stencil
#	Sources/SwiftGenCLI/templates/files/structured-swift4.stencil
#	Sources/SwiftGenCLI/templates/files/structured-swift5.stencil
#	Sources/TestUtils/TestsHelper+Context.swift
#	SwiftGen.xcodeproj/project.pbxproj
#	Tests/TemplatesTests/TestsHelper.swift
@djbe djbe enabled auto-merge April 12, 2021 02:59
@djbe djbe modified the milestones: Next minor, 6.5.0 Apr 12, 2021
@djbe djbe disabled auto-merge April 12, 2021 03:07
@djbe djbe merged commit 7681672 into SwiftGen:develop Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants