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

Run more tests on both JVM/JS #309

Merged
merged 4 commits into from
Mar 31, 2022
Merged

Run more tests on both JVM/JS #309

merged 4 commits into from
Mar 31, 2022

Conversation

armanbilge
Copy link
Contributor

fs2-io supports all APIs (including files) on JVM and JS now.

@satabin
Copy link
Member

satabin commented Mar 30, 2022

Hi @armanbilge and thanks for the contribution. One first question: why did you move all the main sources from shared to src. Sources in shared is used on bith JVM and JS platforms, so this should be no problem.

@armanbilge
Copy link
Contributor Author

All the sources are shared now, so you can simplify your directory structure to an ordinary project (no need for shared/jvm/js split).

build.sbt Show resolved Hide resolved
@satabin
Copy link
Member

satabin commented Mar 30, 2022

All the sources are shared now, so you can simplify your directory structure to an ordinary project (no need for shared/jvm/js split).

The main sources were already shared, but for sake of uniformity, I’d like to keep all modules use the shared structure, if that’s ok for you.

@armanbilge
Copy link
Contributor Author

Sure no problem!

@satabin
Copy link
Member

satabin commented Mar 30, 2022

@armanbilge
Copy link
Contributor Author

Yes, actually it seems there are many more tests that can cross-compile now. Looking through quickly I see the CsvParserTest can also run on JS now.

@armanbilge armanbilge changed the title Run all xml tests on JVM/JS Run more tests on both JVM/JS Mar 30, 2022
@armanbilge
Copy link
Contributor Author

armanbilge commented Mar 30, 2022

I think the only module that actually needs split sources is csv, because URL is not supported on Scala.js. It looks to me like everything else 100% shares sources (JSON will be able too as well now.)

@satabin
Copy link
Member

satabin commented Mar 30, 2022

Ok thanks I need to do a global check on my use of shared, I thought I had more specific cases.

@ybasket
Copy link
Collaborator

ybasket commented Mar 30, 2022

I let two of you figure out the sbt sources setup, it's by far not my strength 🙄

But thank you very much for the work @armanbilge! Feel free to ping me on the Typelevel Discord (same handle) if you need more workflow approvals on the way.

build.sbt Show resolved Hide resolved
Copy link
Collaborator

@ybasket ybasket left a comment

Choose a reason for hiding this comment

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

Looks good from my side then, but I'll let @satabin do the merge in case there's something with the source folders still to be optimised.

@satabin
Copy link
Member

satabin commented Mar 31, 2022

Looks good from my side then, but I'll let @satabin do the merge in case there's something with the source folders still to be optimised.

Nah, it’s fine, I’ll merge it. Thanks a lot @armanbilge!

@satabin satabin merged commit 3385105 into gnieh:main Mar 31, 2022
@satabin satabin added this to the 1.4.0 milestone Jun 3, 2022
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