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

Combine common directories. #197

Closed
pixelzoom opened this issue Feb 27, 2024 · 5 comments
Closed

Combine common directories. #197

pixelzoom opened this issue Feb 27, 2024 · 5 comments
Assignees
Labels
dev:code-review dev:help-wanted Extra attention is needed

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 27, 2024

Related to #32 ...

I understand from implementation-notes.md that VSM is “Variability, Sources, and Measures screens”. And I suspect that SM is “Sources and Measures screens”, though it's not mentioned in implementation-notes.md. That said...

Most repos have a single common/ directory, with the convention that anything used in > 1 place goes in common/. And in fact the CRC says:

For a multi-screen sim, code shared by 2 or more screens should be in a js/common/ subdirectory.

This sim is a little odd in that it has no common/, but has common-vsm/ and common-sm/. This approach feels unnessary and not generally scalable. If you had something only used in Variability and Sources screens, would be created a 3rd common directory, common-vs/ ? And if something is shared by Variability and Sampling screen, then what? -- you've already used common-vs. You also have file prefixes "VSM" and "SM" that seem to identify related screens.

So... I recommond combining common-vsm/ and common-sm/ into common/. You might want to solicit a couple of other opinions on this.

@pixelzoom
Copy link
Contributor Author

In Slack#DM, @samreid said:

That was intentional and we felt it was preferable to have a separate partition for the code only used in the VSM screens. We extended that for the (more minimal but still important to keep separate) changes for the SM screens.

@pixelzoom pixelzoom mentioned this issue Feb 27, 2024
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Feb 27, 2024

Subdirectories under a single common/ directory would probably accomplish what you want. But “vsm”, “sm”, etc. needs to be spelled out somewhere.

If you decide to keep common-vsm and common-sm in #197, or move them to subdirectories of common/, you might say something about them in implementation-notes.md (correspondence to "VSM" and "SM", what value they are providing, etc.)

@samreid
Copy link
Member

samreid commented Feb 27, 2024

@matthew-blackman let's discuss how to proceed.

@samreid
Copy link
Member

samreid commented Mar 7, 2024

@matthew-blackman and I deleted the "sm" folder and inlined that into VSM in the commit.

We tried moving the vsm model and view under common/ like so:

image

But neither of us liked it.

We don't want to introduce another layer of nesting in the common directory. We also like that each directory has "model/" and "view/". So we feel the best option is to have:

common/
  model/
  view/
common-vsm/
  model/
  view/

I'll document this in implementation notes since it is nonstandard and doesn't satisfy the code review document.

samreid added a commit that referenced this issue Mar 7, 2024
@samreid
Copy link
Member

samreid commented Mar 7, 2024

We updated the documentation, closing.

@samreid samreid closed this as completed Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev:code-review dev:help-wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants