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

Remove RMG-Java code and fix various related glitches #281

Merged
merged 8 commits into from
Jul 25, 2024
Merged

Remove RMG-Java code and fix various related glitches #281

merged 8 commits into from
Jul 25, 2024

Conversation

jonwzheng
Copy link
Contributor

Motivation

We decided several months ago that RMG-Py will no longer support any RMG-Java functionality (see ReactionMechanismGenerator/RMG-Py@9f293be).

This means that RMG-website also will not be able to support those functions, unless some of the functions are ported over. Indeed, the save_html_file that was removed from RMG-Py was used in some of the RMG-website tools.

This PR

To reduce the possibility of further error, and to be consistent with RMG-Py's philosophy, this PR removes any code related to RMG-Java. It also fixes a couple of related errors:

  • updates the Chemkin code to use an RMG-website version of save_html_file rather than trying to import this removed function from RMG-Py
  • add blank=True, null=True to many of the model forms which appear to be required(or at least a workaround) for getting the tools to work (see here; this is slightly undesirable behavior, as now the page can be submitted without uploading any files, but this shouldn't cause any significant issues)

Side note

This is not the first time that removal of deprecated RMG-Py code affected RMG-website downstream. We may want to consider including the RMG website build into the RMG-Py CI workflow, but I think this would be too slow/perhaps unnecessary.

Another option is to set up CI for RMG-website; that way we can catch these errors at least during PRs and during regular build tests. (See #280)

Copy link
Contributor

@JacksonBurns JacksonBurns left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me! Thanks for cleaning up this tech debt.

Feel free to merge at your lesiure

@jonwzheng jonwzheng merged commit 20b7c24 into main Jul 25, 2024
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.

2 participants