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

lib: fail gracefully if we can't find the username #2375

Merged
merged 2 commits into from
May 19, 2021

Conversation

DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Apr 12, 2021

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

In lib\find-python.js...

  • Only attempt to get os.userInfo().username in a try {} block, since it's not 100% guaranteed to succeed
  • If we can't find the Appdata\Local directory, skip checking paths under Appdata\Local.

Fixes #2373

DeeDeeG added 2 commits April 12, 2021 08:43
Don't run this outside of a try block, as it can fail in some scenarios,
especially some containerization or virtual system scenarios
with no user ID or username.
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Apr 12, 2021

Notes for reviewers:

Logging: I hoped to add logging to this, but I couldn't think of a way to do that without adding a new logging function at the file-level scope (outside of PythonFinder), and hoisting the errorLog[] array to the file-level scope as well. If that much of a re-write of lib/find-python.js is acceptable for a simple bug-fix PR, I can submit that [EDIT: see link in bullet point below]. But for now this is geared more toward being a minimalist fix rather than a maximalist/perfectionist/potentially over-eager one.

  • A refactor with getWinDefaultLocations() in its own function, with more logging and comments: comparison/diff

Stop trying to use os.userInfo() altogether?: Rather than getting os.userInfo.username in a try {} block, we could delete that bit of the code. And as I did in this PR already, skip the paths under AppData if we don't get the information we need (but stopping at reading from env vars, not running os.userInfo()). I'm not really sure if os.userInfo() is worth the trouble or not.

@owl-from-hogvarts
Copy link
Contributor

@DeeDeeG hi. As i see, you are working on find-python.js too. My opinion is that as we working on same the piece of repo, we should strongly collaborate to avoid doing same work twice as it is now)) A am already have started to refactor logging system in my fork but have not pushed commits yet

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented May 6, 2021

Hi @owl-from-hogvarts.

I believe this PR applies cleanly over your utf-8 branch, and vice-versa; There should be no merge conflicts here. (I did make a branch with a lot of refactoring and logging, but I actually think that much change is inappropriate for a bug fix PR. And I am not pursuing that much refactoring until some of the backlog of open, recent PRs is addressed some.)

Yes, coordinating is a good idea if multiple people are working on the same file in parallel. But I am not planning to work on this repo for now, until some open pull requests get merged or closed. I don't think there is a point to contribute with a backlog of open PRs that might land, because it does indeed make it harder to predict which ones will conflict. And because it has actually been over a month since anything was landed at this repository, I am not sure when/if PRs will be merged. I don't want to propose a lot of changes, and potentially have to remember what I was working on months later to address feedback from reviewers. So for now I am not planning on submitting any additional PRs, until the backlog of recent open PRs is addressed. So I hope that means you don't have to worry about conflicts from my branches.

Best regards.

P.S. I understand most Node folks are volunteers and have a lot of various responsibilities. This is in no way a complaint or critique. I just want to put this out in addressing @owl-from-hogvarts's suggestion of coordination, and avoiding conflict between PRs. I think as I'm not posting new PRs it shouldn't be an issue. And it's a moot point until there is the real chance things start getting merged again.

P.P.S. I am sorry that my other PR #2333 caused merge conflicts for your UTF-8 PR.

@owl-from-hogvarts
Copy link
Contributor

Wow! thanks for so detailed answer 😄

@gengjiawen gengjiawen merged commit fca4795 into nodejs:master May 19, 2021
@DeeDeeG DeeDeeG mentioned this pull request Jun 21, 2021
4 tasks
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.

Issue with non 'username' defined
4 participants