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

DOC: Add a "User messaging" design doc #7310

Merged
merged 11 commits into from May 12, 2023

Conversation

jsheunis
Copy link
Member

@jsheunis jsheunis commented Mar 1, 2023

This PR:

TODOs:

  • @datalad/developers: all please review the suggested design, since it will set the specification for future changes to the code base (+ extensions), as well as refactoring efforts
  • Examples to be added to help developers apply the design specifications to their own extension code or to core contributions
  • A description of the UI module and its use to be added
  • Add changelog entry

@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.15 🎉

Comparison is base (4c9d834) 90.61% compared to head (4736a88) 90.76%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7310      +/-   ##
==========================================
+ Coverage   90.61%   90.76%   +0.15%     
==========================================
  Files         324      325       +1     
  Lines       43034    43244     +210     
  Branches        0     5766    +5766     
==========================================
+ Hits        38994    39252     +258     
+ Misses       4040     3976      -64     
- Partials        0       16      +16     

see 120 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jsheunis jsheunis changed the base branch from maint to master March 1, 2023 15:37
Copy link
Member

@adswa adswa left a comment

Choose a reason for hiding this comment

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

Thanks a lot for writing this up, this summarizes a lot of principles and guidelines really well. I left a few comments. Overall, I would agree with almost everything you outlined here, and I'm also curious what @yarikoptic and @mih say.

docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Show resolved Hide resolved
@adswa adswa added semver-documentation Changes only affect the documentation, no impact on version CHANGELOG-missing When a PR's description does not contain a changelog item, yet. labels Mar 1, 2023
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Mar 2, 2023
Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up. I think it will need a few iteration to bring us all on the same page. I have left a bunch of comments.

In general, I'd prefer if we try to find the strongest language that we can. I fear that with shoulds and mays, we and up with something that is hard to use as an actionable specification.

docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
------------------

In general, **exceptions should be raised when there is no way to ignore or recover from
the offending action**.
Copy link
Member

Choose a reason for hiding this comment

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

IMHO This is not an optimal statement, but I cannot come up with a better one, quickly.

Copy link
Member

Choose a reason for hiding this comment

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

If this statement is concerning only Interfaces implementations, then I would agree. But then there is no clarity on either it concerns those or any part of the code, e.g. a function, which might be just compute_y(x) and which

  • if just a regular classical Python function -- would raise ValueError or TypeError exception
  • if somehow is "an interface" -- yield "impossible" record.

how does developer to decide between the two?

Overall -- is this document specifically just about subclasses of Interfaces or only the first section on return records is?

Copy link
Member Author

Choose a reason for hiding this comment

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

Overall -- is this document specifically just about subclasses of Interfaces or only the first section on return records is?

I think it's both. The main theme is user messaging, and the reality is that most of that happens in response to a call via the Interface class. But also: we aren't giving general advice i.e. it should not be seen as a comment on what to do with general/classic Python code.

docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Some thoughts, haven't finished review though so may be some are premature

docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
@jsheunis
Copy link
Member Author

Note: the PR is still in draft because of the section Examples that hasn't been added. This would contain code examples of how to use each user messaging method. However: I would like to know opinions on whether this is actually necessary? I'm leaning towards it being a nice-to-have, but not strictly necessary.

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

Thx for taking that on, @jsheunis!

docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Show resolved Hide resolved
jsheunis and others added 3 commits March 19, 2023 15:20
Co-authored-by: Benjamin Poldrack <bpoldrack@users.noreply.github.com>
Co-authored-by: Benjamin Poldrack <bpoldrack@users.noreply.github.com>
Co-authored-by: Benjamin Poldrack <bpoldrack@users.noreply.github.com>
@jsheunis
Copy link
Member Author

@datalad/developers thanks for the comments so far. I must still add minor updates after the review of @bpoldrack. And then I would like input on whether we need examples or not? And lastly, a last sanity check on the latest state would probably be a good idea, for those who care strongly about the content here.

@jsheunis
Copy link
Member Author

Comments by @yarikoptic in a dev call: examples could point to commit-tracked implementations in existing code.

@jsheunis jsheunis marked this pull request as ready for review March 28, 2023 07:59
@codeclimate
Copy link

codeclimate bot commented Apr 18, 2023

Code Climate has analyzed commit fc28212 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

quickly spotted the NameError: name 'command' is not defined so left some suggestions ;-)

docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
docs/source/design/user_messaging.rst Outdated Show resolved Hide resolved
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@jsheunis
Copy link
Member Author

@yarikoptic thanks, I committed the suggestions

@yarikoptic
Copy link
Member

Thank you @jsheunis for pushing it through!

@yarikoptic yarikoptic merged commit 8b3ee73 into datalad:master May 12, 2023
19 of 21 checks passed
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.19.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-documentation Changes only affect the documentation, no impact on version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants