drmohundro / SWXMLHash

Simple XML parsing in Swift
MIT License
1.4k stars 203 forks source link

Draft idea for post deserialization validation #251

Closed woolie closed 3 years ago

woolie commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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/action-swiftlint@3.2.1
        with:
          args: --strict
codecov[bot] commented 3 years ago

Codecov Report

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

:exclamation: 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%) :arrow_down:

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 commented 3 years ago

@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 commented 3 years ago

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.