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

add developer documentation and design principles #72

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

Conversation

s9105947
Copy link

New section "Developer Documentation" is aimed at developers wanting to
work on PICMI itself. Its content may be of interest to implementors,
but should not be required knowledge to them.

The design principles outlines fundamental design decisions and aims to
clarify misunderstandings on PICMI at an early stage.


Key points that are important to me personally:

  • Users are not allowed to access attributes directly.
  • Implementing codes use the same attributes as the users, but may be offered additional representations of the same data (e.g. laser E0/a0)
  • Implementing codes receive a integrity guarantee from the check interface.

New section "Developer Documentation" is aimed at developers wanting to
work on PICMI itself. Its content may be of interest to implementors,
but should not be required knowledge to them.

The design principles outlines fundamental design decisions and aims to
clarify misunderstandings on PICMI at an early stage.
@s9105947 s9105947 changed the title add developer documentation and design principles [WIP] add developer documentation and design principles Mar 18, 2022
@s9105947
Copy link
Author

Note: I am not that familiar with the rst format, feel free to make suggestions.

@s9105947 s9105947 changed the title [WIP] add developer documentation and design principles add developer documentation and design principles Mar 22, 2022
@s9105947
Copy link
Author

I've proofread and would put considers this document "done", at least for the basics.

I summon @dpgrote @ax3l @BrianMarre for their opinions!

ax3l
ax3l previously approved these changes Sep 8, 2022
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Please excuse the delay.

This looks great to me. Tiny formatting inline.

@dpgrote @RemiLehe can we merge this?

Docs/source/developer/design_principles.rst Outdated Show resolved Hide resolved
Docs/source/developer/design_principles.rst Outdated Show resolved Hide resolved
@ax3l ax3l added the Documentation Add or update documentation and comments label Sep 8, 2022
@ax3l
Copy link
Member

ax3l commented Sep 8, 2022

Users are not allowed to access attributes directly.

I have no problem with that point. The only thing we should note is that writing attributes directly can invoke undefined behavior (inconsistent objects if not all corresponding members are updated). Reading is actually no issue, unless we want to hide unit conversions within.

Copy link
Member

@dpgrote dpgrote 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 good to me.

@ax3l
Copy link
Member

ax3l commented Sep 9, 2022

@s9105947 @BrianMarre just a minor question to clarify above, if you have a chance :)

@BrianMarre
Copy link
Member

BrianMarre commented Sep 11, 2022

Yes it probably is a good idea to note that writing to attributes causes undefined behaviour, and yes I agree that reading should not cause problems, as in both will probably not cause and should under no circumstances cause problems by design.

Edit: I did change my mind somewhat, we should discourage the user from needlessly reading variables but it should not be disallowed in general


Note that even if not recommended direct access to member variables
of objects is still possible.
Accessing the member variables directly (both reading and writing) results in **undefined behavior**.
Copy link
Member

Choose a reason for hiding this comment

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

I think that not encouraging read access, is justified, since read-to-set-other-variable creates a lot of implicit dependencies difficult to oversee

Copy link
Member

@ax3l ax3l Sep 12, 2022

Choose a reason for hiding this comment

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

agreed, we can recommend using getters instead

Copy link
Member

Choose a reason for hiding this comment

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

@BrianMarre @s9105947 want to update accordingly? :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but I am on vacation right now and Hannes moved on.
I am actually not sure wether I even have write access to this pull request.
Might be that we have to get a developer to push to this pull request or, create a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Add or update documentation and comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants