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

Draft idea for post deserialization validation #251

Merged
merged 7 commits into from
May 15, 2021

Conversation

woolie
Copy link
Contributor

@woolie woolie commented Apr 6, 2021

The remaining issue is how to provide validation on the built in types and whether that is important.
Right now it is easy to add an override for the type you are implementing deserialization for, but I'm not sure how you would do the build ins like Int, Double, String without adding the validation on the type that has those inside of them.

I only added tests the BasicItem and nothing for [BasicItem] nor attributes, but the support code is there and can be added if people like the idea.

@woolie
Copy link
Contributor Author

woolie commented Apr 7, 2021

Not sure why I am getting nesting listing errors with the GitHub Action but I can't repro locally. I am using the latest SwiftLint.

@drmohundro
Copy link
Owner

@woolie code looks great, thanks! I'll see if I can figure out what is going on with the linter - it might just be the GitHub Actions version.

@drmohundro
Copy link
Owner

I'm hoping that if the .github/workflows/lint.yml file is updated with the following (just a bump to 3.2.1) it will resolve the lint issues:

name: SwiftLint

on:
  [push, pull_request]

jobs:
  docker-lint:
    name: Docker Lint
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v1
      - name: GitHub Action for SwiftLint with --strict
        uses: norio-nomura/[email protected]
        with:
          args: --strict

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #251 (77e970b) into main (42480d9) will decrease coverage by 0.39%.
The diff coverage is 65.65%.

❗ Current head 77e970b differs from pull request most recent head 82f04ed. Consider uploading reports for the commit 82f04ed to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
- Coverage   74.74%   74.34%   -0.40%     
==========================================
  Files          13       14       +1     
  Lines        1829     1910      +81     
==========================================
+ Hits         1367     1420      +53     
- Misses        462      490      +28     
Impacted Files Coverage Δ
...SWXMLHashTests/TypeConversionBasicTypesTests.swift 68.50% <ø> (ø)
...LHashTests/TypeConversionPrimitypeTypesTests.swift 67.39% <ø> (ø)
Tests/SWXMLHashTests/XMLParsingTests.swift 84.23% <ø> (ø)
...sts/SWXMLHashTests/XMLParsingValidationTests.swift 64.00% <64.00%> (ø)
Source/XMLIndexer+XMLIndexerDeserializable.swift 68.94% <66.21%> (-0.68%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42480d9...82f04ed. Read the comment docs.

@woolie
Copy link
Contributor Author

woolie commented Apr 12, 2021

@drmohundro so I up'ed the lint version as you suggested, I also updated the swift version to current and updated the simulator to the current as well. All of those changes made things pass.

@drmohundro drmohundro self-requested a review May 15, 2021 21:38
@drmohundro drmohundro merged commit 8fe82e1 into drmohundro:main May 15, 2021
@drmohundro
Copy link
Owner

Sorry for taking a few weeks to get it merged... looks great to me. Next steps to get it fully released are to get docs and a version bump. I'm thinking about including #246 in this, too, and then doing a full version bump. That PR is about a rename so that the primary class doesn't match the module name... some weird SPM issue I think.

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