Skip to content

Conversation

@pouyapd
Copy link

@pouyapd pouyapd commented May 9, 2025

This pull request integrates a new crate, scan_fmt_scxml, to add SCXML parsing capabilities to the SCAN project.

Changes include:

  • Added the scan_fmt_scxml crate containing the SCXML parsing logic.
  • Modified scan_scxml/src/parser.rs to use the scan_fmt_scxml library for parsing .scxml files.
  • Updated root Cargo.toml to include scan_fmt_scxml in the workspace and dependencies.
  • Updated scan_scxml/Cargo.toml to add scan_fmt_scxml as a dependency for the scan_scxml crate.

This allows SCAN to load models specified in the SCXML format.

@EnricoGhiorzi EnricoGhiorzi self-requested a review May 10, 2025 13:24
@EnricoGhiorzi
Copy link
Collaborator

Dear @pouyapd thank you for the PR. There are a few issues with it:

  • The commit has no "sign-off" which is why is failing the DCO test. You need to sign-off all of your commits to attest you are submitting your code according to the repo licence. Please look at online documentation about this git sign-off functionality and how to fix your current commit.
  • The code does not build because scan_fmt_scxml has no Cargo.toml manifest. I haven't tried building past this point, so I don't know if the rest works.
  • You are importing scan_fmt_scxml into scan_scxml as a dependency, which should not be required. On the contrary, you might want to import scan_scxml into scan_fmt_scxml to use the builder functionalities.
  • The PR is composed by a single massive commit (by the way, that's not best practice, but I am fine with it). The real issue is that this way your colleagues do not appear as contributors. Please ensure that all of you push some commits (with sign-off) so that you will appear as contributors in the repo.

Please fix the above issues so that I can move on with the review process.

@pouyapd
Copy link
Author

pouyapd commented May 11, 2025

Dear Professor,

I have reviewed your feedback on this Pull Request.

I have made the following corrections:

  • Fixed the DCO sign-off issue. The latest commit is now correctly signed.
  • Added the missing Cargo.toml manifest file for the scan_fmt_scxml crate.
  • Corrected the dependency in `scan_scxml/Cargo.toml.
  • The automated workflow is now running (or waiting for approval) and should pass the basic build check related to Cargo.toml files and sign-off.

Thank you again for your clear feedback!

Best regards,

Copy link
Collaborator

@EnricoGhiorzi EnricoGhiorzi left a comment

Choose a reason for hiding this comment

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

There are issues with the scan_fmt_scxml/Cargo.toml file that prevent building. Please fix these so that I can move on with the review.

@@ -0,0 +1,24 @@
[package]
name = "scan_scxml"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name conflicts with the other scan_scxml. I suggest renaming your package and library crate as scan_fmt_scxml.

edition = "2024"

[lib]
name = "scan_scxml" # The name of the target.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as previous comment.

chumsky = "0.10.1"
csv = { workspace = true }
flate2 = { workspace = true }
scan_fmt_scxml = { path = "../scan_fmt_scxml" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

A package cannot have itself as a dependency. You maybe wanted to import scan_scxml? Not sure it's needed anyway.

@pouyapd
Copy link
Author

pouyapd commented May 13, 2025

Dear Professor,
Following up on Pull Request #36. The DCO check has passed.
The automated Rust build workflow is currently still pending approval on GitHub before it can run. Please let me know if you are able to approve it so I can see the build results.

Atoosa Ebrahimabadi and others added 4 commits May 14, 2025 23:26
@EnricoGhiorzi EnricoGhiorzi self-requested a review May 20, 2025 11:39
Copy link
Collaborator

@EnricoGhiorzi EnricoGhiorzi left a comment

Choose a reason for hiding this comment

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

Dear SCXML group, I have reviewed the PR and I found some issues:

  • The code does not compile: there are issues on the Cargo.toml manifests breaking the compilation. Did you forgot to include some commits maybe?
  • I made the code compile and run the tests with cargo test. I found out that the tests in scxml_lib do not run because there is no associated test.rs file. Please include such a file (you can take as example the relevant code in the other packages) so that your code can be automatically tested.

pouyapd added a commit to pouyapd/scan that referenced this pull request Jun 2, 2025
Copy link
Collaborator

@EnricoGhiorzi EnricoGhiorzi left a comment

Choose a reason for hiding this comment

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

There are still errors that are preventing build. One I pointed out. Another is the chumsky dependency being updated to 0.10 while the code is still using 0.9. Other errors are due, I believe, to merging the main branch into yours. The main branch ofter gets breaking changes so I reccommend not to merge it (unless you make sure to also fix your code accordingly).

I recommend to not merge upstream main into your branch, and to make sure your code compiles. If your code is compiling locally, it means it is not aligned with the PR.


[dependencies]
scan_core = { version = "0.1.0", path="../scan_core" }
scan_fmt_scxml = { path = "../scan_fmt_scxml" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes to this files are unnecessary (as you don't need to modify scan_scxml) and also break building, because the line was added twice. Please fix this.

pouyapd added a commit to pouyapd/scan that referenced this pull request Jun 3, 2025
@pouyapd pouyapd closed this Jun 11, 2025
pouyapd added a commit to pouyapd/scan that referenced this pull request Jun 11, 2025
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.

4 participants