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

Modifications to RiverFlowDynamics #1

Merged

Conversation

mcflugen
Copy link

@mcflugen mcflugen commented Oct 20, 2024

Description

@angelmons, I've been going through landlab#1979 (your RiverFlowDynamics component) and it looks great! Thank you!

I began to make suggestions for some rather minor changes but figured it might be easier for me to just go ahead and make them myself. As such, I thought I would try something a little different than we normally do.

If you're ok with it, my plan is to use this pull request as a base for some of these changes for you to review. I'll break this PR into several smaller ones to make them easier for you to review.

What do you think?

Checklist - did you ...

  • Add a news fragment file entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?
  • All tests have passed?
  • Formatted code with black?
  • Removed lint reported by flake8?
  • Sucessful documentation built? (if documentation added or modified)

@mcflugen mcflugen marked this pull request as draft October 20, 2024 21:07
@angelmons
Copy link
Owner

angelmons commented Oct 21, 2024

Description

@angelmons, I've been going through landlab#1979 (your RiverFlowDynamics component) and it looks great! Thank you!

I began to make suggestions for some rather minor changes but figured it might be easier for me to just go ahead and make them myself. As such, I thought I would try something a little different than we normally do.

If you're ok with it, my plan is to use this pull request as a base for some of these changes for you to review. I'll break this PR into several smaller ones to make them easier for you to review.

What do you think?

Checklist - did you ...

  • Add a news fragment file entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?
  • All tests have passed?
  • Formatted code with black?
  • Removed lint reported by flake8?
  • Sucessful documentation built? (if documentation added or modified)

Hi @mcflugen, Thank you so much!
Yes, please feel free to edit anything you see fit and proceed with your plan. I greatly appreciate your help and support.

@coveralls
Copy link

@angelmons angelmons marked this pull request as ready for review October 21, 2024 01:28
@angelmons angelmons merged commit d62b368 into angelmons:riverFlowDynamics_dev Oct 21, 2024
43 of 44 checks passed
@mcflugen
Copy link
Author

Hi @mcflugen, Thank you so much! Yes, please feel free to edit anything you see fit and proceed with your plan. I greatly appreciate your help and support.

@angelmons Sounds good. Thanks! You weren't supposed to merge this pull request yet, though! 😄 I guess if it looked ok to you, then it's all good. I'll likely open up another one.

@mcflugen mcflugen deleted the mcflugen/river_flow_dynamics branch October 21, 2024 01:49
@angelmons
Copy link
Owner

Hi @mcflugen , hahaha, I am sorry. I was trying to solve the problem with the changelog entry.

I will wait this time. Please let me know when it is ready to review.

Thanks again.

@angelmons
Copy link
Owner

angelmons commented Oct 22, 2024 via email

@mcflugen
Copy link
Author

@angelmons I'm still going through it but am swamped at the moment so it's taking longer than usual.

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.

3 participants