Skip to content

add full name calculation#1

Draft
gregbunce wants to merge 27 commits intomainfrom
feat/road-name-calculation
Draft

add full name calculation#1
gregbunce wants to merge 27 commits intomainfrom
feat/road-name-calculation

Conversation

@gregbunce
Copy link
Copy Markdown

just getting things going, so i wanted to check in before i got too far.

@steveoh steveoh changed the title am i on the right track? add full name calculation Dec 10, 2020
Copy link
Copy Markdown
Contributor

@steveoh steveoh left a comment

Choose a reason for hiding this comment

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

yeah this looks like you're on the right track

Comment thread .vscode/settings.json Outdated
Comment thread src/rules/common.py Outdated
Comment thread src/rules/roads.py Outdated
@steveoh steveoh marked this pull request as draft December 10, 2020 17:50
@gregbunce
Copy link
Copy Markdown
Author

i made a few changes based on your suggestions. should i mark it as 'ready for review'?

@gregbunce gregbunce marked this pull request as ready for review December 14, 2020 15:56
@steveoh steveoh self-requested a review December 14, 2020 18:02
@steveoh
Copy link
Copy Markdown
Contributor

steveoh commented Dec 14, 2020

Can you send me or better yet commit the xml schema doc so I can load in the db and test this?

@steveoh steveoh self-assigned this Dec 14, 2020
@gregbunce
Copy link
Copy Markdown
Author

gregbunce commented Dec 23, 2020

let me know if you need some data as well. i exported the 'schema only'
image

@gregbunce
Copy link
Copy Markdown
Author

also, the xml schema contains all the needed layers (data store) for the attribute rules. let me know if you only want the roads layer (the layer we're putting the attribute rules on).
image

@steveoh
Copy link
Copy Markdown
Contributor

steveoh commented Dec 24, 2020

also, the xml schema contains all the needed layers (data store) for the attribute rules. l

The schema imported but when I tried to append data to some of the datasets so the rules will function the schema's didn't match and it failed. I ended up having to delete and then copy in the data. Let's do a schema export with data to eliminate these steps.

Copy link
Copy Markdown
Contributor

@steveoh steveoh left a comment

Choose a reason for hiding this comment

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

This seems to be in a draft state still but going in the right direction.

There are quite a few issues that are frustrating for a reviewer and I will list them so we can have more productive reviews in the future.

  1. The pr text is a bit light. It says you're hoping to be going in the right direction but don't really state the known issues or things you are working out.
  2. You don't mention what the full name calculation is expected to do. We don't have tests yet so some text on expectations would be very helpful.
  3. The ar.py wasn't able to run and add the rules for testing.
  4. The pro project was missing with the connection sde file.
  5. The roads_edit fc was not able to have attribute rules added to it since it is missing global ids.

Otherwise yes it looks like you aren't in the weeds making timely mistakes.

@steveoh steveoh marked this pull request as draft December 24, 2020 21:59
@gregbunce
Copy link
Copy Markdown
Author

gregbunce commented Dec 30, 2020

thanks for the useful feedback and for the work you put into this pr to get it up and running. all good points - and noted on my end.

I'll pull this branch to my local repo. then...

  1. add globalid to roads_edit
  2. replace the existing XML schema with one containing the data.
  3. test your commits
  4. push my local branch back to the remote
  5. 'ready for review'

Does that sound right?

thanks again. once i get this set up, i feel as though I'll be able to make more timely progress on the project - doing local testing and a more focused pr.

should i do my local testing using the pro project you added to the repo?

@steveoh
Copy link
Copy Markdown
Contributor

steveoh commented Dec 30, 2020

You bet. This branch should already exist in your local repo unless you did all the work in the github.com ui which I doubt. So you can most likely just checkout feat/road-name-calculation if you're not already on it and git pull.

  1. yes
  2. yes
  3. yes and make the necessary changes to make sure the rules are creating the proper results
  4. the commits you make to get the rules properly functioning, yes, you will then push to this existing branch. no need to make any new branches
  5. yes - when everything is working as expected

Yes we should align on that pro project. There might be some broken items, i'm thinking paths etc but we should work that out so we are working on identical items.

@gregbunce
Copy link
Copy Markdown
Author

gregbunce commented Dec 30, 2020

k, perfect. yup, i pulled your changes to my local branch.

i'm trying to work out the pro project now. working on how to use the shared sde connection file. is that possible? it looks like you used os auth.

is there a way we can use a common Instance name or should i just use my local connection string and just work on sharing the pro project? (if that makes sense)

here's how i see your connection file:
image

@steveoh
Copy link
Copy Markdown
Contributor

steveoh commented Dec 30, 2020

let's try to go with the local style as that will work for anyone with a local sql server and won't have any username/passwords in it.

this is now working in arcgis pro testing
this is now working in arcgis pro testing
and set triggers to update; also rename ar rules to reflect the associated field name
@gregbunce
Copy link
Copy Markdown
Author

gregbunce commented Jan 22, 2021

all of the calculation rules appear to be working in arcgis pro during my manual testing. the next step is to now write some testing in test_roads.py. i put some time aside next week to do so (beginning Monday).

also, currently, we have one constrain rule that we've created (predir_domain_constraint) but there are no domains in the db. how should we handle that? i guess they didn't carry over in the XML schema. i can export the existing domains in utrans (the 'real' utrans) to tables and then we can add them to the local utrans db. does that work?

Copy link
Copy Markdown
Contributor

@steveoh steveoh left a comment

Choose a reason for hiding this comment

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

looks like you're on the right track!

Comment thread pro-project/.pyHistory
@@ -0,0 +1,18 @@
table = "Roads_Edit"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

chore: add this file to the .gitignore please

Comment thread readme.md

```sh
conda create --clone arcgispro-py3 --name utrans
conda create --name utrans
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

praise: niiiiiice!

Comment thread tests/test_roads.py
Comment on lines +15 to 16
sde = os.path.join(os.path.dirname(__file__), '..', 'pro-project', 'localhost@utrans.sde')
TABLE = os.path.join(sde, table_name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: we should switch this to use the newer pathlib which is the recommended library for the future.

something like the following should suffice

Suggested change
sde = os.path.join(os.path.dirname(__file__), '..', 'pro-project', 'localhost@utrans.sde')
TABLE = os.path.join(sde, table_name)
from pathlib import Path
sde = Path(__file__).parent / '..' / 'pro-project', 'localhost@utrans.sde'
TABLE = str(sde / table_name)

Comment thread tests/test_roads.py
with arcpy.da.UpdateCursor(TABLE, ['OID@'], where_clause="FacilityName IN ('0', '1')") as cursor:
for _ in cursor:
cursor.deleteRow()
#arcpy.management.TruncateTable(TABLE)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

chore: we need to figure out the reset issue otherwise the subsequent tests will fail

@steveoh steveoh assigned gregbunce and unassigned steveoh May 14, 2024
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.

2 participants