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

Past officers #285

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

Past officers #285

wants to merge 2 commits into from

Conversation

jyxzhang
Copy link
Contributor

@jyxzhang jyxzhang commented Oct 3, 2021

A proposed schema for past officers (and current if we want I guess). Ideally, the new OCF static website would read from this (and maybe some other things if we want).
OCFWeb reads historical officers from here. Note that only OCF username is stored; it then uses OCF lib to get the name and such. In my opinion, we can just store the name too, but discussion is welcome.

In this schema, we have an array of term objects, which have a termSemester as well as termOfficers. TermOfficers is an object that includes gms, sms, deputies, committee heads, each of which is an pastOfficer. pastOfficers have a handle, first name, last name, start and end dates (will be used for officers that served for parts of semesters), and committee (will only be used for committee heads).

I'm not too familiar with schemas, so hopefully things aren't super scuffed. Definitely looking for feedback!

Note: The YAML for this schema has not been created yet. I was hoping to nail down the schema so we wouldn't have to maybe rewrite the YAML if the schema gets changed because of feedback.
To create the YAML, the best way would probably to do some scraping.
@Joelrodiel did some work on past officers on the static and scraped some JSON here ocf/ocfstatic#205, but it is missing committees and doesn't match the exact structure. It seems like a good starting point though.

Copy link
Member

@dkess dkess 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 working on this!

While it's not necessary to fully write out the real YAML config, since the schema is still a draft (as you wrote), but it would be useful to write out a small subset (probably one of the semesters where there were lots of leadership changes, like fall 2018) so we can see what the YAML looks like and if it looks natural to read/write.

In general, it's easier to start with example YAML, then write a schema after the fact.

@@ -0,0 +1 @@
{}
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this file

@@ -0,0 +1,6 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this file

"type": "array",
"items":
{
"$ref": "#/$defs/pastOfficer"
Copy link
Member

Choose a reason for hiding this comment

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

This isn't ideal because we have to repeat the same pastOfficer object every time someone has a position, which means we'll have to repeat their full name multiple times. Ideally we'd only need to write someone's full name once.

Let's do something similar to staff hours (https://github.com/ocf/etc/blob/master/configs/staff_hours.yaml), where we only specify the username here (i.e. make this a string), and at the bottom of the file have a separate section that maps username to full name.

I'm imagining something like this:

semester: Fall 2018
officers:
  gm:
    - handle: asai
      start: 2018-05-12
      end: 2018-11-07
...

Then at the bottom of the file,

fullNames:
  asai: "Andrew Aikawa"

"$ref": "#/$defs/pastOfficer"
}
},
"gmsDeputies":{
Copy link
Member

Choose a reason for hiding this comment

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

nit: the title is actually "Deputy GM", so this should be written as "deputyGMs" (same for SMs).

"officerHandle": {
"type": "string"
},
"officerFirstName": {
Copy link
Member

Choose a reason for hiding this comment

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

Don't have separate "first name" and "last name" fields. It's best to have one "full name" field.

(I don't have a good source for this piece of advice, so you'll have to trust me that it's best practice.)

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 since people's names can be sometimes don't fit just the first/last mold (from Dutch tussenvoegsels to multiple last names) generally just using a full name is best.

"officerLastName"
],
"properties": {
"officerHandle": {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: as with before, no need to prefix each property name with officerHandle, it's unnecessarily verbose since we know it's for an officer.

"type": "array",
"items":
{
"$ref": "#/$defs/pastOfficer"
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't say which committee they're head of. You're including that in pastOfficer, which is awkward because it's not a meaningful property to add to the SM/GM/deputies lists. It also means the committee names are getting repeated more than necessary.

This should probably be an object that looks something like:

committeeHeads:
  "Internal":
    - handle: jaw
    - handle: ronitnath
...

It can also be just a list of strings if we don't expect this to be as complicated as SMs with start/end dates.

"termOfficers"
],
"properties": {
"termSemester": {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: no need to prefix these properties with term, just do semester.

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.

None yet

4 participants