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

PullConcreteStrengthForBarsFromEtabs #211

Merged
merged 2 commits into from
Dec 9, 2019
Merged

Conversation

RichardWhitfieldTTW
Copy link

@RichardWhitfieldTTW RichardWhitfieldTTW commented Oct 2, 2019

Issues addressed by this PR

Added read of concrete strength for bars in ETABS.
Amended assembly name for adapter.
Pluralised some method names for improved descriptiveness.

Closes #220

Does not fix any existing issues.

Question: The assembly name is amended based on the build configuration but does this break the ability of the adapter to be found by reflection? Since this was outside of the scope of my feature I did not amend the csproj file for this and manually renamed the dll after the build.

@al-fisher al-fisher added the type:feature New capability or enhancement label Oct 2, 2019
Copy link
Contributor

@JosefTaylor JosefTaylor left a comment

Choose a reason for hiding this comment

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

Can you leave the pluralization of methods to another PR? That requires some discussion.

@JosefTaylor
Copy link
Contributor

Hey @RichardWhitfieldTTW, @RichardWhitfield, we're excited to have you helping out with this; will you keep working on this PR or should we log this as an issue, close the PR, and come back to it later?

@RichardWhitfieldTTW
Copy link
Author

Hi Josef,
Apologies, I let this slip. Let me close this out this week.
Rich

Copy link
Contributor

@IsakNaslundBh IsakNaslundBh left a comment

Choose a reason for hiding this comment

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

@RichardWhitfieldTTW @RichardWhitfield

To get this merged we will keep only the changes made to the read of material and revert back the rest. Will revert any stylistic changes made to the PR (method names from plural to singular) especially as a series of other PRs have been merged in already.

Hope you are ok with this! Sorry this has been dragging for a while.

RichardWhitfield and others added 2 commits December 9, 2019 10:07
Amended assembly name.
Pluralised some method names for improved discriptiveness.
@FraserGreenroyd FraserGreenroyd force-pushed the ConcreteMaterialPullFix branch from f2b8b47 to f81f242 Compare December 9, 2019 10:08
@IsakNaslundBh
Copy link
Contributor

Check in Etabs manual:

image

Extracting as cylinder strength looks to be correct.

@al-fisher al-fisher dismissed their stale review December 9, 2019 10:27

Thanks guys - happy addressed original comments! Thanks again @RichardWhitfield - good to get this additional functionality finally merged!

Copy link
Contributor

@IsakNaslundBh IsakNaslundBh left a comment

Choose a reason for hiding this comment

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

LGMT now. Pulls Cylinder strength correctly from my checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ETABS_Toolkit: Read concrete strength
5 participants