Skip to content

Conversation

@D0wn10ad
Copy link

Fixes:

Copy link
Contributor

@Timpan4 Timpan4 left a comment

Choose a reason for hiding this comment

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

Thank you for your PR!

Overall the feature implementation looks good. Just some small suggestions.

Could you also rebase and fix the merge conflicts before approval?

Comment on lines 60 to 68
```Commnad prompt
echo "PHAB_TOKEN: cli-ABC123" > %LOCALAPPDATA%\phabfive\phabfive.yaml
```

**Environment variables:**
```bash
export PHAB_TOKEN=cli-ABC123
export PHAB_URL=https://yourserver.com/api/
Additionally, due to connection to Phabricator server on HTTPS requires certificate verification, it is also recommended to install [pip_system_certs](https://pypi.org/project/pip-system-certs/) to ensure system store are linked to python.

```Commnad prompt
pip install pip_system_certs
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change the codeblock type to shell and the pip install command to uv pip install

Copy link
Author

Choose a reason for hiding this comment

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

hi, as this is windows specific, should I change to shell?

Copy link
Contributor

Choose a reason for hiding this comment

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

The Windows specific ones for the CMD "Command Prompt" would be batch, but I would set this particular one to shell and the two above (with %SOMEVAR%) to batch as they reference variables in the CMD syntax.

It would however be best to convert the Windows examples to Powershell and use powershell instead.

Comment on lines 134 to 135
# Extended support for phabricator instances with more than 100 (i.e. deftault query limit) users
# BUT with limieted exception handling e.g. retries needed when phabricator instance is not responding to API calls
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments could be removed. They only serve a purpose for the reviewer of this PR

Comment on lines 151 to 152
# Extended support for phabricator instances with more than 100 (i.e. deftault query limit) projects
# BUT with limieted exception handling, e.g. retries needed when phabricator instance is not responding to API calls
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments could be removed. They only serve a purpose for the reviewer of this PR

Removed comments regarding extended support for fetching users and projects from Phabricator.
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