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

Create new library RPA.Smartsheet #880

Merged
merged 25 commits into from Mar 23, 2023
Merged

Create new library RPA.Smartsheet #880

merged 25 commits into from Mar 23, 2023

Conversation

kylie-bee
Copy link
Contributor

@kylie-bee kylie-bee commented Mar 10, 2023

Create the new library RPA.Smartsheet. This includes the new way we can import __version__. This will need to be added to other packages via a follow on PR.

To do list:

  • Get Python tests to passing (all are and will be mocked).
  • Merge master into this branch.
  • Update documentation, add examples
  • Write release notes
  • Build and release

Keywords included in initial release:

  • Set access token
  • Set max retry time
  • Get application constants
  • List sheets
  • Unselect current sheet
  • Get sheet
  • Convert sheet to table
  • Get sheet owner
  • List sheet filters
  • Create sheet
  • Search
  • List columns
  • Add columns
  • Add column
  • Update column
  • Convert row to dict
  • Get row
  • Set rows
  • Set row
  • Add rows
  • List attachments
  • Download attachment
  • Get current user
  • Get cell history

* Set access token
* Set max retry time
* Get application constants
* List sheets
* Unselect current sheet
* Get sheet
* Convert sheet to table
* Get sheet owner
* List sheet filters
* Create sheet
* Search
* List columns
* Add columns
* Add column
* Update column
* Convert row to dict
* Get row
* Set rows
* Set row
* Add rows
* List attachments
* Download attachment
* Get current user
@kylie-bee kylie-bee self-assigned this Mar 10, 2023
@cmin764 cmin764 added the feature New functionality to add label Mar 14, 2023
@cmin764 cmin764 added this to In progress in RPA Framework via automation Mar 14, 2023
@kylie-bee kylie-bee marked this pull request as ready for review March 15, 2023 17:45
@kylie-bee
Copy link
Contributor Author

Tests are failing only because google.com is blocking our test traffic with a reCaptcha.

Copy link
Member

@cmin764 cmin764 left a comment

Choose a reason for hiding this comment

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

Review comments below:

packages/main/pyproject.toml Outdated Show resolved Hide resolved
packages/main/src/RPA/Smartsheet.py Outdated Show resolved Hide resolved
packages/main/src/RPA/Smartsheet.py Outdated Show resolved Hide resolved
packages/main/src/RPA/Smartsheet.py Outdated Show resolved Hide resolved
packages/main/src/RPA/Smartsheet.py Show resolved Hide resolved
packages/main/src/RPA/Smartsheet.py Outdated Show resolved Hide resolved
packages/main/src/RPA/Smartsheet.py Outdated Show resolved Hide resolved
packages/main/src/RPA/Smartsheet.py Outdated Show resolved Hide resolved
packages/main/src/RPA/Smartsheet.py Outdated Show resolved Hide resolved
packages/main/src/RPA/Smartsheet.py Show resolved Hide resolved
Copy link
Member

@cmin764 cmin764 left a comment

Choose a reason for hiding this comment

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

Thanks for the huge effort of bringing this in, still, besides a few cosmetics I've discovered some bugs and other issues that we may need to improve before releasing it.

Post-review PR

Outside of the inline comments, here are some other matters:

  1. We need a README entry as well describing this library.
  2. I saw no docs being able to be generated for RPA.Smartsheet (with inv docs.* commands in my branch review/library/smartsheet) -- also take note of the invocations.docs.EXCLUDES constant as well.
  3. Make sure you're consistent with using Camel Case Keyword format in examples/tests. (the default behavior of exposed keywords)

Nice to have

How hard is to setup a test drive where we can run in CI a few basic sweet-case RF robot tests, testing end-to-end for real the service? (the mocks of unit-tests won't be able to let us know if there's something fundamentally wrong with the library usage when accidentally introducing a regression in the future)

PyCharm can discover/warn most of these

Screenshot 2023-03-16 at 15 15 23

Screenshot 2023-03-16 at 15 04 28

cmin764 added a commit that referenced this pull request Mar 16, 2023
@kylie-bee
Copy link
Contributor Author

kylie-bee commented Mar 16, 2023

@cmin764 What linter or otherwise is PyCharm running to report those problems? I would think integrating that into our linters would be the best thing to do, second best is integrate that into VSCode so one does not need to install a whole IDE just to check those problems...

As for the Camel Case default, I remember seeing in slack some time ago that preferred was now First letter capitalization?

Docs was on my list to build and then I got sidetracked just getting the actual documentation into the code and never built them, I'll need to build them tomorrow. I've responded to most if not all of your comments and resolved those that were straight forward or I agreed with. Thank you for the thorough review!

@cmin764
Copy link
Member

cmin764 commented Mar 17, 2023

@cmin764 What linter or otherwise is PyCharm running to report those problems? I would think integrating that into our linters would be the best thing to do, second best is integrate that into VSCode so one does not need to install a whole IDE just to check those problems...

Good question, and here's an unofficial answer. So most probably we use the same linters, just that our configuration is less strict to not make CI a smartass dictator. But this doesn't mean we have to ignore such warnings/problems, as misleading type-annotations might be worse than no types at all.

PyCharm was built by Py devs for Py devs and it comes with its own IDE effort to help you write error-proof idiomatic Python code faster. If we use the right tool for the job, which is developing a Python library, the time spent to install, learn & use such new IDE might be less than the time spent in reviews & refactoring on both sides.

As for the Camel Case default, I remember seeing in slack some time ago that preferred was now First letter capitalization?

I'm aware just of this recent debate, where it was suggested the opposite. But please let me know if there's any newer contradiction to this.

Docs was on my list to build and then I got sidetracked just getting the actual documentation into the code and never built them, I'll need to build them tomorrow. I've responded to most if not all of your comments and resolved those that were straight forward or I agreed with. Thank you for the thorough review!

You welcome and thank you for addressing them (or explaining the decision/opinion behind)!

And add `isort` to `pyproject.toml` dev dependencies and set isort's profile to `black`
@kylie-bee
Copy link
Contributor Author

Alright, cleared everything and built the docs without error. I did not have a chance to test the logging nor did I have time to test it with a live robot yet. I must switch projects and cannot devote much more time to this unfortunately.

@cmin764 cmin764 self-requested a review March 20, 2023 04:56
Copy link
Member

@cmin764 cmin764 left a comment

Choose a reason for hiding this comment

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

LGTM! (although real robot tests are missing -- we can address missing stuff in a follow-up PR post-merge)

@kylie-bee
Copy link
Contributor Author

One check failed due to Google blocking our traffic.

@kylie-bee kylie-bee merged commit 226ac2b into master Mar 23, 2023
11 of 12 checks passed
RPA Framework automation moved this from In progress to Waiting for release Mar 23, 2023
@kylie-bee kylie-bee deleted the library/smartsheet branch March 23, 2023 18:56
kylie-bee added a commit that referenced this pull request Mar 23, 2023
@cmin764 cmin764 moved this from Waiting for release to Done in RPA Framework Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality to add
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants