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

Integrating MAYGEN project to structgen package #801

Closed
wants to merge 2 commits into from
Closed

Integrating MAYGEN project to structgen package #801

wants to merge 2 commits into from

Conversation

javadev
Copy link
Contributor

@javadev javadev commented Jan 12, 2022

No description provided.

@johnmay
Copy link
Member

johnmay commented Jan 12, 2022

Sorry I have this on the branch locally, the dependance on commons-cli needs to go - also (I have made that change). I also suspect cdk-group isn't needed but need to dig deeper. If you split out the SDF change to a separate PR I can pull that in right away.

@javadev
Copy link
Contributor Author

javadev commented Jan 12, 2022

@johnmay here your are #802

@javadev
Copy link
Contributor Author

javadev commented Jan 28, 2022

Sorry I have this on the branch locally, the dependance on commons-cli needs to go - also (I have made that change). I also suspect cdk-group isn't needed but need to dig deeper. If you split out the SDF change to a separate PR I can pull that in right away.

commons-cli dependency was excluded from the pull request. cdk-group is used in class Maygen.

@johnmay
Copy link
Member

johnmay commented Jan 30, 2022

This fix isn't quite right since the fields are private there is no way to configure it. Basically to go in to CDK it needs to have an actually API and not some opaque executable invocation. I also really don't like it handling the IO writing, makes sense for a command line - not for a modular API.

I just wanted to start working on this now but got sidetracked by stupid xerces issues.

@javadev
Copy link
Contributor Author

javadev commented Jan 30, 2022

It may be added to cdk and improved latter. I think real customers will use a command line application.

@johnmay
Copy link
Member

johnmay commented Jan 30, 2022

But the point is we don't currently have any command line :-). So it should just be it's own project.

@javadev
Copy link
Contributor Author

javadev commented Jan 30, 2022

I hope it will be merged soon.

@MehmetAzizYirik
Copy link
Contributor

This fix isn't quite right since the fields are private there is no way to configure it. Basically to go in to CDK it needs to have an actually API and not some opaque executable invocation. I also really don't like it handling the IO writing, makes sense for a command line - not for a modular API.

I just wanted to start working on this now but got sidetracked by stupid xerces issues.

@johnmay we already had the initial pull request and we formatted that as Egon's recommended. Just to share my thoughts: we can re-open the initial pull request and close this one. Then doing the changes on that clean version. CDK has its own format and that is why I did not wanna do any changes after sharing with you. Rather than doing these changes and asking whether it is correct or not for CDK, with your guidance we can work on the clean version.

@egonw
Copy link
Member

egonw commented Jan 30, 2022

But the point is we don't currently have any command line :-). So it should just be it's own project.

Some many moons ago "we" made the wise decision to not have command line utilities in the main library and removed the CLI wrappers we had at the time (yes, in a long past we did have CLI tools).

We should start a project to have these CLI tools again. Besides the MAYGEN CLI tool, we'll have a (SDF, SMILES) file validator, our own babel, etc.

@johnmay
Copy link
Member

johnmay commented Feb 5, 2022

Now #811

@johnmay johnmay closed this Feb 5, 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.

None yet

4 participants