Skip to content

Comments

Rewrite for Swift#48

Open
fulldecent wants to merge 15 commits intoArthurGuibert:masterfrom
fulldecent:main
Open

Rewrite for Swift#48
fulldecent wants to merge 15 commits intoArthurGuibert:masterfrom
fulldecent:main

Conversation

@fulldecent
Copy link
Contributor

Thank you for this great library!

I have refaced this for Swift. And Swift PM. And have implemented best practices for Swift modules from https://github.com/fulldecent/swift6-module-template

@ArthurGuibert
Copy link
Owner

Hey @fulldecent, thanks for the pull request! I didn't expect this for this fairly old library. Do you think it's still relevant now that we have Swift Charts from Apple directly? Anyways, happy to merge this if it's useful for you.
I left a few comments to be addressed if you have the time for this. If not, I'll at the PR a bit more thoroughly and fix the things myself.

All notable changes to this project will be documented in this file.
FSLineChart adheres to [Semantic Versioning](http://semver.org/).

## [Main](https://github.com/fulldecent/FSLineChart)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
## [Main](https://github.com/fulldecent/FSLineChart)
## [Main](https://github.com/ArthurGuibert/FSLineChart)

If we merge this PR in this repo we should keep the urls consistant


## Release process

1. Confirm the build is [passing in GitHub Actions](https://github.com/fulldecent/FSLineChart/actions)
Copy link
Owner

Choose a reason for hiding this comment

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

Same comment here, about the url of the repo

2. Push a release commit
1. Create a new Main section at the top
2. Rename the old Main section like:
## [1.0.5](https://github.com/fulldecent/FSLineChart/releases/tag/1.0.5)
Copy link
Owner

Choose a reason for hiding this comment

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

dito

@@ -0,0 +1,34 @@
import XCTest

final class ExampleUITests: XCTestCase {
Copy link
Owner

Choose a reason for hiding this comment

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

No big deal, but if it's not used we could remove the tests files

limitations under the License.

Copyright (c) 2019–2025 William Entriken
Copyright (c) 2019 Yaroslav Zhurakovskiy
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Copyright (c) 2019 Yaroslav Zhurakovskiy
Copyright (c) 2019 Yaroslav Zhurakovskiy
Copyright (c) 2014-2019 Arthur Guibert

It's a change to MIT right? If so I don't mind 👍

cancel-in-progress: true

# Keep this up to date with https://github.com/FSLineChart/FSLineChart/blob/main/.github/workflows/ci.yml
# And also https://github.com/Alamofire/Alamofire/blob/master/.github/workflows/ci.yml
Copy link
Owner

Choose a reason for hiding this comment

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

Why is Alamofire mentioned? This shouldn't be a dependency or related at all

with:
languages: swift
- name: Build macOS
run: set -o pipefail && env NSUnbufferedIO=YES xcodebuild -project "Alamofire.xcodeproj" -scheme "Alamofire macOS" -destination "platform=macOS" clean build | xcpretty
Copy link
Owner

Choose a reason for hiding this comment

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

Don't we need to change the reference to FSLineChart instead of Alamofire here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is that7e2d0729154d8f8d2fd61ddef0062e11316ebfe0

But really I have no idea to make testing work.

We can delete that part if it is not doing anything helpful

@fulldecent
Copy link
Contributor Author

I have been using it so I thought the least I can do is send a PR upstream.

Yes, swift charts looks cool too. But what's FS is designed in it's just designed in :-)

@fulldecent
Copy link
Contributor Author

All of the changes you mentioned look good. Yes you can add them in or I can add them in.

Regarding Alamo fire that is a typo from my CI script that I'm using. I'm having trouble getting CI to work.

And my notes about that are at swift6-module-template.

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.

3 participants