Skip to content

Conversation

@padiro
Copy link
Contributor

@padiro padiro commented Feb 28, 2025

Hey Yannick,

here is my suggestion for PySpectre v0.0.4.

This pull request enhances the codebase by providing two concrete implementations of an abstract interface that standardizes session management. The abstract interface defines a clear contract of methods and attributes, ensuring both implementations adhere to the same API. The dummy implementation offers a simulated session setup, enabling rapid testing and development without relying on a live backend. In contrast, the real implementation manages an actual session with comprehensive command construction, file verification, and error handling for robust production use. Together, these changes improve modularity, facilitate easier testing, and streamline future enhancements.

Regards,
Pau

@AugustUnderground
Copy link
Owner

Hi Pau,

thanks for the suggestion. This PR adds over 1000 lines of code. I'm not sure PySpectre needs this kind of complexity. I'm not sure what the purpouse of this OOP refactoring is either. Can you give me a use-case for this dummy interface? Maybe that'll help me understand. What other possible implementations could there be that would use this interface?

I've conceptualized PySpectre to be a minimalist and barebones library for running simulations with spectre. Anything beyond that should be application- or user specific. That means creating a separate project that imports pyspectre and builds the required functionality around it. So, if you need this kind of functionality in your application, the intended use would be to import pyspectre as it is and use it only in the corresponding class.

I strongly beleive that nothing beyond what's absolutly essential for communicating with spectre from python should be in this core library. Everything else should be built around it in separate projects and code bases.

@padiro
Copy link
Contributor Author

padiro commented May 7, 2025

From my side, the PR would be ready to merge.

@AugustUnderground
Copy link
Owner

Thank you! Yes, this looks really good. I think this is a pretty major change to the module and deserves a corresponding version bump to 0.1.0 what do you think?

@padiro
Copy link
Contributor Author

padiro commented May 12, 2025

Since the project is stable enough and significant new features have been added, I would agree.
In the last commit, I updated the changelog.md and setup.py.

@AugustUnderground AugustUnderground merged commit f465585 into AugustUnderground:master May 13, 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.

2 participants