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

Printer to print local children of imported components #591

Merged
merged 50 commits into from
Aug 13, 2020

Conversation

kerimoyle
Copy link
Contributor

@kerimoyle kerimoyle commented May 5, 2020

Addresses #590.

Additional to this test, I'd also expect to be able to specify this relationship (equivalence between imported variables) through the API. This isn't possible at the moment?

Also closes #609

tests/variable/variable.cpp Outdated Show resolved Hide resolved
@hsorby
Copy link
Contributor

hsorby commented May 6, 2020

To be clear it is possible to do this at the moment.

@kerimoyle
Copy link
Contributor Author

So is this a thing? What should I do in the tutorials? Use dummy variables or wait for an overloaded function?

@nickerso
Copy link
Contributor

nickerso commented May 6, 2020

What should I do in the tutorials? Use dummy variables or wait for an overloaded function?

Either push ahead with the dummy variable method in the tutorial (this works now) or implement the proposed new API for the convenience methods and use them in the tutorials.

Personally, I'm not sure how much of a convenience that these methods would be so I'd continue on the tutorials with the dummy variables.

@kerimoyle
Copy link
Contributor Author

I'm going to revert the second commit, as discussion about workarounds there has obscured the actual bug ... I'll add the other as a "nice to have maybe" issue on its own.

@kerimoyle kerimoyle changed the title Test showing printer missing equivalent variables Test showing **Parser** missing equivalent variables and encapsulations May 27, 2020
@kerimoyle kerimoyle changed the title Test showing **Parser** missing equivalent variables and encapsulations Printer to print local children of imported components Jun 1, 2020
@kerimoyle
Copy link
Contributor Author

Wow, this PR went all over the show! In the end the bug was in the Printer, not the Parser as I'd originally thought. This PR has three aspects:

  • Fixed the bug in the printer that was missing local children of imported components (src/printer.cpp). This is the main bit!
  • Improved the debug printing of the model structure (test_utils.cpp)
  • Added tests for printing/parsing of imports, encapsulations, and connections (tests and resources files)

Ready for review please @agarny @hsorby @nickerso

hsorby
hsorby previously requested changes Aug 12, 2020
tests/test_utils.cpp Outdated Show resolved Hide resolved
tests/variable/variable.cpp Outdated Show resolved Hide resolved
tests/variable/variable.cpp Outdated Show resolved Hide resolved
@kerimoyle kerimoyle requested a review from hsorby August 13, 2020 01:22
@kerimoyle kerimoyle dismissed hsorby’s stale review August 13, 2020 01:28

addressed and made changes

hsorby
hsorby previously approved these changes Aug 13, 2020
@agarny
Copy link
Contributor

agarny commented Aug 13, 2020

This PR seems to be broken now.

@hsorby
Copy link
Contributor

hsorby commented Aug 13, 2020

Just minor API change required I think.

hsorby
hsorby previously approved these changes Aug 13, 2020
Copy link
Contributor

@agarny agarny left a comment

Choose a reason for hiding this comment

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

Looks good apart from a few minor requests.

@kerimoyle kerimoyle requested a review from hsorby August 13, 2020 07:01
@hsorby hsorby merged commit 84ebec3 into cellml:develop Aug 13, 2020
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.

Feature or bug? Local component child of imported parent not printed?
4 participants