Skip to content

Conversation

@YooSunYoung
Copy link
Collaborator

It's just extracting some lines to a separate function.

I was doing it while I was trying to understand the logic.
It was more easy to read before, but I added some more detailed error messages and intermediate state to make debugging easier.

  • type hinting.
  • hiding some interface that is not expected to be used out side of the init function.

@mads-bertelsen
Is there a reason why you keep the file object instead of just reading relevant informations all at once?
I think most of the information can be just loaded at the beginning and kept it as properties.

@mads-bertelsen
Copy link
Contributor

Good updates!

@YooSunYoung the decisions about how much of the file the package loads automatically and what level of control the user has over the file open/close come from how I imagine the package will typically be used. I think most users will explore data file a few times, figuring out what to load and so forth, and then in future runs just load exactly what they need. So in that initial exploration phase I want the package to be able to provide overview / help to the user, and in the second phase I want it to be as simple/fast as possible so it doesn't get in the users way.

The decision to have the main class a context manager was to give the user control of opening / closing of the file, and only load minimal overview information automatically. The rest is explicitly requested at this time. I think this is reasonable design for these base classes, but we could easily add an abstraction layer around it that use these classes and provide an interface more suited for that exploration phase where more information is loaded and shown automatically, and file handling is hidden.

@mads-bertelsen mads-bertelsen merged commit 3d591f5 into mcstas-version-config Jun 3, 2025
4 checks passed
@YooSunYoung YooSunYoung deleted the nexus-file-obj-init branch June 3, 2025 09:17
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