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

Fixing with a workarround the problems of caixa-enginyers with swift 6 #37

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

Dracks
Copy link
Owner

@Dracks Dracks commented Jan 8, 2025

Summary by CodeRabbit

  • New Features

    • Added a workaround for decoding Windows CP-1252 encoded files in Swift.
    • Improved file parsing with enhanced string encoding handling.
  • Bug Fixes

    • Implemented a fallback mechanism for string decoding across different operating systems.
  • Chores

    • Improved test formatting and consistency in profile tests.

Copy link
Contributor

coderabbitai bot commented Jan 8, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

The pull request introduces a Windows CP-1252 encoding workaround in Swift, specifically addressing file parsing and string conversion challenges. The changes span multiple files, focusing on adding a robust mechanism to handle Windows CP-1252 encoded files across different operating systems. The modifications include a new encoding mapping, a custom string conversion method, and updates to the HTML file parsing logic to leverage this new encoding support.

Changes

File Change Summary
src/swift-server-tests/profile.tests.swift Minor formatting adjustments in testCreateUserAsAdmin method, no functional changes
src/swift-server/core/swift-6-latin-workarround.swift Added Windows CP-1252 encoding mapping and custom string decoding method
src/swift-server/importer/importers/file-parser.swift Updated parserHtml function to use new Windows CP-1252 encoding conversion method

Sequence Diagram

sequenceDiagram
    participant FileParser
    participant StringEncoding
    participant FileData
    
    FileParser->>StringEncoding: Check encoding type
    alt Windows CP-1252 Encoding
        StringEncoding->>FileData: Use custom decoding method
    else Other Encodings
        StringEncoding->>FileData: Use standard encoding conversion
    end
    FileData-->>FileParser: Return decoded string
Loading

Poem

🐰 A Swift tale of bytes and might,
Where Windows encoding takes flight!
CP-1252, no longer a pain,
Our rabbit decoder breaks the chain 🔤
Decoding magic, line by line! 🌟


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c9bbdb and 56c056d.

📒 Files selected for processing (2)
  • src/swift-server/core/swift-6-latin-workarround.swift (1 hunks)
  • src/swift-server/importer/importers/file-parser.swift (1 hunks)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/swift-server/importer/importers/file-parser.swift (1)

15-21: Consider enhancing error handling for encoding failures.

The current implementation throws a generic E10007 exception for all encoding failures. Consider differentiating between Windows CP-1252 and other encoding failures to aid in debugging.

 switch encoding {
 case .windowsCP1252:
   fileString = String.fromWindowsCP1252(fileData)
+  guard let fileString else {
+    throw Exception(.E10007, context: [
+      "fileName": filePath,
+      "encoding": "Windows CP-1252"
+    ])
+  }
 default:
   fileString = String(data: fileData, encoding: encoding)
+  guard let fileString else {
+    throw Exception(.E10007, context: [
+      "fileName": filePath,
+      "encoding": String(describing: encoding)
+    ])
+  }
 }
-guard let fileString
-else {
-  throw Exception(.E10007, context: ["fileName": filePath])
-}
src/swift-server/core/swift-6-latin-workarround.swift (2)

1-4: Enhance header documentation.

Consider adding more context about the issue being worked around and when this workaround can be removed.

-// Workarround to be able to decode windowsCP1252 files
-// thanks to @TimBudarick
-// To be remove when the following issue is fixed: https://github.com/swiftlang/swift/issues/78382
+// Temporary workaround for Windows CP-1252 encoding support in Swift
+// This implementation provides a manual decoder for Windows CP-1252 encoded files
+// as Swift's built-in support is currently incomplete.
+//
+// Credits: @TimBudarick
+// 
+// TODO: Remove this workaround when Swift adds proper Windows CP-1252 support
+// Tracking issue: https://github.com/swiftlang/swift/issues/78382

276-292: Consider performance optimization for large files.

The current implementation builds the string by repeatedly appending characters, which can be inefficient for large files. Consider pre-allocating the string capacity or using a more efficient string building approach.

 static func fromWindowsCP1252(_ data: Data) -> String? {
   #if os(macOS)
     return String(data: data, encoding: .windowsCP1252)
   #else
-    var decodedString = ""
+    // Pre-allocate capacity to avoid repeated reallocations
+    var decodedString = String()
+    decodedString.reserveCapacity(data.count)
 
     for byte in data {
       if let character = String.Encoding.windowsCP1252Map[byte] {
         decodedString.append(character)
       } else {
         return nil
       }
     }
 
     return decodedString
   #endif
 }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2aa3564 and 5c9bbdb.

📒 Files selected for processing (3)
  • src/swift-server-tests/profile.tests.swift (1 hunks)
  • src/swift-server/core/swift-6-latin-workarround.swift (1 hunks)
  • src/swift-server/importer/importers/file-parser.swift (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/swift-server-tests/profile.tests.swift
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Test
🔇 Additional comments (1)
src/swift-server/core/swift-6-latin-workarround.swift (1)

8-265: Well-implemented character mapping!

The Windows CP-1252 mapping is comprehensive and well-documented. Good practice using the Unicode replacement character (U+FFFD) for undefined code points.

@Dracks Dracks merged commit 50b628d into mr-scrooge-3.0.0 Jan 8, 2025
1 of 4 checks passed
@Dracks Dracks deleted the fix/swift-6-workarround branch January 8, 2025 15:16
Dracks added a commit that referenced this pull request Mar 26, 2025
* wip: Full refactor of the server to swift with vapor

* fixing the test-swift pipeline

* [CodeFactor] Apply fixes

* Fixing the codefactor fix

* Rules & rule-engine backend completed in swift (#30)

* updating the client to fix stuff and implementing admin of the users

* updating the client to fix stuff

* Adding admin users

* Fixing coderabbit comments

* dropping old view

* preparing the creation of users

* User admin and almost all linting fixed

* Fixing with a workarround the problems of caixa-enginyers with swift 6 (#37)

* Fixing with a workarround the problems of caixa-enginyers with swift 6

* Some small suggestions to improve the code

* Create a migration command to migrate the data from V2 to V3 (#38)

* Create a migration command to migrate the data from V2 to V3

* Fixing some of coderabbit comments

* some small improvements

* some small improvements

* Adding a transaction for the migration

* Add the rules and all this into the UI (#43)

* Working on the UI from rules

* Small improvements on the transaction-row

* Run format and lint

* Advancing on the edit rule form

* Runing tests for the create label and edit/delete it

* Adding a delete functionality of the rule

* small improvements from coderabbit

* Adding cache to swift (to test something for the work) (#42)

Co-authored-by: Jaume <[email protected]>

* Change description to comment field (#45)

* Adding enums into the schemas, and this implied a huge refactor to allow postgres to work (#46)

* Modifying the docker, only x86 for the moment (#47)

* Github action multiplatform (#48)

* Github action multiplatform

* Trying different things

* Fixing small things of the frontend

* Adding cache and updating all actions, let's pray to broke as less as possible!

* fixing migration service (#50)

* fixing migration service

* Testing to run the tests in arm

* Removing the paralel on the swift-tests and see if fixes the pipeline

* Improve migration with sorting (#53)

* fix migration error on tags to rules transformation (#54)

* disable review in draft P/Rs for coderabbit

* Sorting the second level in graphs, that should improve the output graphs (#57)

* Adding the missing labels functionality (#56)

* Adding the missing labels functionality

* Improving the labels, with the option to force the drop of the label no matter if is linked somewhere

* Last small changes

* Added an improvement suggestion of coderabbit

* Some small minor improvements

* fix some issue

* Fixing the pipeline! (#60)

* Fixing the pipeline!

* fixing the cache step

* Limiting the reloads triggered by the bank transaction and other small things (#61)

* Limiting the reloads triggered by the bank transaction and other small things

* fixing pipeline

* Small improvements

* Removing all the stuff from the v2 (#63)

* Doing a huge cleanup of the repo

* Update docker/docker-compose.sqlite.yml

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Adding a configuration class that loads all the env vars. (#64)

* Adding a configuration class that loads all the env vars.

* Remove a print

* Reworking Ids in tables and other stuff (#65)

* Reworking Ids in tables and other stuff

* Fixing small things

* Fixing and adding more tests

* Minor changes

* Update todo.md

* Things that needs to be fixed in alpha-4 (#66)

* Things that needs to be fixed in alpha-4

* improve docs, and remove the constraints in the environment on the frontend

* Handling the sorting of the graphs (#67)

* Handling the sorting of the graphs

* Applying some minor changes

* fixing the issue

* fixing the tests

* Added a security feature to not allow brute force attacs to log-in (#70)

* Added a security feature to not allow brute force attacs to log-in

* Apply suggestions from code review

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Added some log to raise the awarnes of too many attempts

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* [CodeFactor] Apply fixes (#71)

Co-authored-by: codefactor-io <[email protected]>

* Some apispec changes in names (#73)

* Some apispec changes in names

* Fixing pipeline

* Add renovate.json (#72)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* New start.sh script (#74)

* New start.sh script

* removing unnecessari nil assignation

* Fixing an error and adding an small improvement

* added some tests

* Adding codecov to get code coverage (#76)

* Adding codecov to get code coverage

* trying to fix the coverage

* Trying new things

* Trying with the codecov action

* trying more things

* Adding curl

* Adding curl

* fixing the token

* Updating stuff

* Added a change password command (#75)

* Added a change password command

* Small things

* Updating the README.md

* try to fix the cache in swift (#77)

* try to fix the cache in swift

* Second hash

* Trying with manual md5

* Fixing the md5

---------

Co-authored-by: codefactor-io <[email protected]>
Co-authored-by: jsinglamms <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

1 participant