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

[Feature request] Formalize/Make-explicit the data structures used throughout org-fc #94

Open
cashpw opened this issue Sep 19, 2022 · 3 comments · Fixed by #102
Open

Comments

@cashpw
Copy link
Contributor

cashpw commented Sep 19, 2022

I had a hard time following the code the first time I read through because the representation of a card/position/etc isn't immediately clear. I expected to find something like org-fc-review-session or a cl-defstruct for each of the nouns used in org-fc (e.g. position, card, index). How do you feel about adding more defclass definitions (e.g. org-fc-position).

Based on what I can tell, those structures could look like:

`org-fc-card`
(defclass org-fc-card ()
  ((created
    :initform nil
    :initarg :created)
   (filetitle
    :initform nil
    :initarg :filetitle)
   (tags
    :initform nil
    :initarg :tags)
   (id
    :initform nil
    :initarg :id)
   (inherited-tags
    :initform nil
    :initarg :inherited-tags)
   (local-tags
    :initform nil
    :initarg :local-tags)
   (path
    :initform nil
    :initarg :path)
   (positions
    ;; List of `org-fc-positions'
    :initform nil
    :initarg :positions)
   (suspended
    :initform nil
    :initarg :suspended)
   (title
    :initform nil
    :initarg :title)
   (type
    :initform nil
    :initarg :type)))
`org-fc-position`
(defclass org-fc-position ()
  ((box
    :initform nil
    :initarg :box)
   (card
    ;; `org-fc-card'
    :initform nil
    :initarg :card)
   (due
    :initform nil
    :initarg :due)
   (ease
    :initform nil
    :initarg :ease)
   (interval
    :initform nil
    :initarg :interval)
   (pos
    :initform nil
    :initarg :pos))
  "Represents a single position.")
`org-fc-review-session-rating`
(defclass org-fc-review-session-rating ()
  ((total
    :intiform 0
    :initarg :total)
   (again
    :intiform 0
    :initarg :again)
   (hard
    :intiform 0
    :initarg :hard)
   (good
    :intiform 0
    :initarg :good)
   (easy
    :intiform 0
    :initarg :easy)))
@l3kn
Copy link
Owner

l3kn commented Sep 19, 2022

I agree that this would help with the readability of the code.

A few years ago I thought about switching the card types to use eieio,
so common code could be shared between cards of different types.
In the case of your defclass org-fc-card with a :type field,
the next logical step would be to skip that :type field and use a subclass for each card type.

I can't remember why I decided against it at that time, here it seems like it could lead to a number of invasive changes, exchanging the learning curve of figuring out which element of a list is which to the learning curve of understanding the details of eieio.

cl-defstruct seems closer to a style already used in some places in org-fc,
a combination of a constructor function that wraps it's arguments in a list
and a set of getter-functions, just automatically defined by a macro.

@l3kn
Copy link
Owner

l3kn commented Sep 19, 2022

Looks like org-fc-review.el already uses defclass
while other files using different styles of wrapper complex data into lists or plists.

In this case my argument about the added learning curve is invalid,
we could just begin with a limited set of eieio functionality and later extend that as we see fit.

@cashpw
Copy link
Contributor Author

cashpw commented Oct 6, 2022

Thank you for merging #102 and #103 into develop! Do you have a timeline or testing plan for graduating develop into main?

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 a pull request may close this issue.

2 participants