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

move under-pressure code to this repo #266

Closed
pixelzoom opened this issue Feb 18, 2015 · 11 comments
Closed

move under-pressure code to this repo #266

pixelzoom opened this issue Feb 18, 2015 · 11 comments

Comments

@pixelzoom
Copy link
Contributor

While working on phetsims/scenery-phet#139, I had problems refactoring fluid-pressure-and-flow, because (a) I wasn't expecting a dependency on another sim, and (b) that dependency was missing from package.json. Figuring out what was going on and refactoring fluid-pressure-and-flow took me longer than all of the other refactoring combined. This was a good illustration of the pitfalls of sim-to-sim dependencies; it creates coupling that is more costly to maintain. And I think it should be avoided unless there is no other alternative.

So is there no other alternative in this case? I don't think so. Some of the things that are being obtained from under-pressure should clearly be moved to common code (BarometerNode, ControlSlider,...) under-pressure.ControlSlider is even documented as "A generic slider control".

I'm going to schedule this issue for discussion in a developer meeting.

@pixelzoom
Copy link
Contributor Author

Here are the things that FPAF is importing from under-pressure:

UNDER_PRESSURE/UnderPressureScreen
UNDER_PRESSURE/common/model/Sensor
UNDER_PRESSURE/common/model/FluidColorModel
UNDER_PRESSURE/common/model/getStandardAirPressure
UNDER_PRESSURE/common/view/ControlSlider
UNDER_PRESSURE/common/view/BarometerNode

The first of these (UnderPressureScreen) tells me that under-pressure is a single screen of fluid-pressure-and-flow. So this entire problem could be made moot by moving under-pressure's source code to fluid-pressure-and-flow. Then the under-pressure project would be just a main that gets its single screen from fluid-pressure-and-flow (as concentration does from beers-law-lab, and ph-scale-basics does from ph-scale). Any reason why we can't do this?

@pixelzoom
Copy link
Contributor Author

2/24/15 dev meeting:
• move under-pressure under FPAF (like concentration/beers-law-lab relationship)
• create issues for things that are good candidates for common-code generalization

I'll do this.

@pixelzoom pixelzoom assigned pixelzoom and unassigned samreid Feb 24, 2015
@pixelzoom
Copy link
Contributor Author

Punting, I'm not going to do this. It's too big a mess, and beyond my pain threshold. The directory structure of under-pressure doesn't match PhET conventions, and requires serious reorganization. And there are things that should be shared between FPAF and Under Pressure that are not being shared (and are instead duplicated) - e.g., ruler, screen view with sky & grass,...

For example, check out the ruler in the first 2 screens, shown below. Totally different orientation!

screenshot_473
screenshot_472

@pixelzoom
Copy link
Contributor Author

Unassigning.

@pixelzoom
Copy link
Contributor Author

This needs to be done before the sim is redeployed, so that translated string lives under the proper repository. See also phetsims/under-pressure#118.

@pixelzoom
Copy link
Contributor Author

Changing title. We determined that this is necessary, and in fact most of the code in the under-pressure repo should live in this repo.

@pixelzoom pixelzoom changed the title is dependency on under-pressure necessary? move under-pressure code to this repo Sep 10, 2015
@zepumph
Copy link
Member

zepumph commented Sep 14, 2017

@ariel-phet phet do we want to do this before deploying FPAF 1.0? If so it seems like the sort of thing that we should do before too long, since it will be a big refactor.

@ariel-phet
Copy link

@zepumph I think Aadish took care of this coupling, you might want to look at commits he made.

@ariel-phet
Copy link

@zepumph or I should say, I think Aadish did some work to make this possible, but yes, we do want to do this work before deploying FPAF 1.0.

@pixelzoom
Copy link
Contributor Author

+1 for doing this before FPAF 1.0.

@zepumph zepumph removed their assignment Mar 26, 2018
@pixelzoom
Copy link
Contributor Author

Looks like this was done on 3/22/2016, but no commits reference this issue. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants