Skip to content

Topology import v0 #136

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Topology import v0 #136

wants to merge 3 commits into from

Conversation

SimonCald
Copy link
Contributor

No description provided.

@SimonCald SimonCald force-pushed the topology_import_V0 branch from 68fa980 to 00bf97e Compare May 20, 2025 08:33
@SimonCald
Copy link
Contributor Author

Pour le moment l'import/export permet le nom conforme et les discrétisation uniforme, progression géométrique et interpolée globales

@SimonCald SimonCald marked this pull request as ready for review May 20, 2025 12:35
@SimonCald SimonCald requested review from lelandaisb and nicolaslg May 20, 2025 12:35
@lelandaisb lelandaisb assigned lelandaisb and unassigned lelandaisb May 23, 2025
@lelandaisb
Copy link
Contributor

lelandaisb commented May 23, 2025

Great new feature
Few comments:

  • export allows you to write a file without extension, whereas import only allows blk extensions. Is it necessary to check on export that the extension is present?
  • the getPreviewRepresentation method can be removed from the ImportBlocksImplementation class, as the command doesn't need it.
  • modern C++ recommends that destructors that do nothing be implemented with the syntax “=0” in the .h file (e.g. ~ImportBlocksImplementation()=0)
  • the “protected” blocks of the ImportBlocksImplementation and ExportBlocksImplementation classes should be replaced by “private”. As long as there is no inheritance, this lets the reviewer know instantly that there are no other uses.
  • Even if the dark mode look is very nice, the QtMgx3DApplication.cpp file must not be included in this PR

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