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

Added badge notice for Atom-Ink and added new badge spec #148

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

confused-Techie
Copy link
Member

Requirements

  • Filling out the template is required.

  • All new code requires tests to ensure against regressions.

    • However, if your PR contains zero code changes, feel free to select the checkmark below to indicate so.
  • Have you ran tests against this code?

  • This PR contains zero code changes.

Description of the Change

This PR adds the badge addition to atom-ink to the admin actions log.

Additionally adding the definition of a new badge type Broken.

The need to add this badge was born from this discussion.


This package likely emits warnings immediately, or may even cause the editor to crash as a whole. Installation of these packages is not recommended by the Pulsar team, and instead it is encouraged to work with the original maintainer to get these packages working, or otherwise the community is encouraged to maintain and manage a fork of said package.

If a community member does decide to maintain a fork of a package with a `Broken` status, it's recommended to make the Pulsar Admins or Pulsar Backend Admins aware of this, so any warnings and links on the original package can be changed to recommend installation of your functional fork.
Copy link
Member

Choose a reason for hiding this comment

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

One small comment is that we don't really define Pulsar Admins or Pulsar Backend Admins anywhere, maybe we should just make this a reference to the Pulsar Team as above or otherwise recommend something like creating an issue in the backend repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good point. Might make the most sense to just refer to the Pulsar Team.

Since not sure making an issue is the right move, although considering how infrequent of an event it'll be might not be that problematic.

@goyalyashpal
Copy link

goyalyashpal commented Apr 14, 2023

I think the atom-ink package is likely on verge of being fixed.

And also, no-one likely installs atom-ink themselves, it gets installed while installing uber-juno package, so ...
Either way (be it fixed, or not), i think badging atom-ink won't be a very good idea....

if anything, uber-juno package should get a badge meanwhile due to its error with Juno Error: Installing julia-client@0.12.6 failed as shown in the image at pulsar-edit/pulsar#482 (comment)

That error is posted in the issue-tracker upstream as well by lots of people, i'll try having a take at it as well, but, 30% it seems unlikely to be fixed to a decent stage

@confused-Techie
Copy link
Member Author

@goyalyashpal I appreciate you contributing on this issue, so we may want to add it to both then, in the unlikely event that someone installs atom-ink` directly.

But appreciate the insight, and do hope that it is properly fixed sometime soon, so we will absolutely keep an eye out for it being done.

But otherwise thanks a ton for your feedback, and I'll edit this when I have the time

Copy link

@goyalyashpal goyalyashpal left a comment

Choose a reason for hiding this comment

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

After both the fixing PRs have been merged, some comments and change requests regarding that.


### Uber-Juno

The [Uber-Juno](https://github.com/JunoLab/uber-juno) package uses an older unsupported coding style, sometimes called JavaScript Sloppy Mode, that's no longer supported in newer versions of NodeJS. Meaning the package crashes as soon as installed, and cannot be used without the user manually editing the source code.

Choose a reason for hiding this comment

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

as per the observation by comment pulsar-edit/pulsar#482 (comment) , it said in that error the following ...

Modules are automatically in strict mode.

and since no issues are shown after that change, I think "it uses Sloppy Mode" no longer applies...


The [Uber-Juno](https://github.com/JunoLab/uber-juno) package uses an older unsupported coding style, sometimes called JavaScript Sloppy Mode, that's no longer supported in newer versions of NodeJS. Meaning the package crashes as soon as installed, and cannot be used without the user manually editing the source code.

One of the Pulsar contributors has [submitted a fix](https://github.com/JunoLab/uber-juno/pull/85) for this issue, although it has not been updated on the Pulsar Package Registry.

Choose a reason for hiding this comment

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

Suggested change
One of the Pulsar contributors has [submitted a fix](https://github.com/JunoLab/uber-juno/pull/85) for this issue, although it has not been updated on the Pulsar Package Registry.
[A fix](https://github.com/JunoLab/uber-juno/pull/85) from one of the Pulsar contributors has been merged, although it has not been updated on the Pulsar Package Registry.

Owing to the mention of "was accepted" in the section for Hydrogen (L22 in the master version)

Comment on lines +15 to +17
Additionally, Uber-Juno relies on Atom-Ink, which is also broken without any fixes, as mentioned below.

Due to these issues of the package, while the `main` branch is fixed, but it relies on broken dependencies, that render's the package as a whole still broken, and in which case will receive a [`broken`](./badge-spec.md#broken) badge.
Copy link

@goyalyashpal goyalyashpal Apr 17, 2023

Choose a reason for hiding this comment

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

Atom-Ink is also fixed.. so, this reason of "broken due to Atom-Ink" will have to be removed, and whole paragraph will have to be reworded to mention that issue with downloading as i mentioned in my previous comment:

Juno Error: Installing julia-client@0.12.6 failed as shown in the image at pulsar-edit/pulsar#482 (comment)

Comment on lines +21 to +27
The [Atom-Ink](https://github.com/JunoLab/atom-ink) package has many coding practices that are no longer valid in JavaScript/NodeJS, such as using reserved words, or using variables without declaring them first, causing the package to crash without manually being edited on Pulsar.

One of the Pulsar contributors has [submitted a fix](https://github.com/JunoLab/atom-ink/pull/289) for this bug, but after four months of inactivity from the maintainer of the package it seems unlikely this will be resolved.

The package is MIT licensed, so if any devs out there would like to pick up the reigns, and get this package functional as a fork, please feel free to do so, and you'd be encouraged to contact the Pulsar Admins about this, so we can update the badges on this package appropriately.

It is for this reason it is **not** recommended to install the `atom-ink` package. It will not work, unless you are comfortable editing the source code of the package manually, or until a community member decides to maintain a fork of said package, which in that event, installing that package would be the recommendation here, rather than not installing.
Copy link

@goyalyashpal goyalyashpal Apr 17, 2023

Choose a reason for hiding this comment

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

This whole will have to be reworded as well... as the issue with using reserved words was with uber-juno, fix for undeclared variable use has been merged.

smth similar to instructions in line 26-28 (section hydrogen) pulsar -p install ... can work but since this is a dependency for uber-juno, i don't know what complications regarding its install that way will be...

also, the upstream hasn't tagged a release yet. so, the command will have to target a commit for the time being...

i have nudged upstream to tag it ...

@Daeraxa
Copy link
Member

Daeraxa commented Apr 17, 2023

@goyalyashpal has the maintainer actually committed to submitting it to Pulsar? Unless they do then we still have the issue of the "bad" version being in the backend and a badge will still need to be applied.

@confused-Techie
Copy link
Member Author

@goyalyashpal I appreciate the review, although we will still probably need to say both packages are broken.

Since uber-juno is essentially a package that just installs other packages. Which one of those is atom-ink. So there is no way to install uber-juno without having atom-ink begin to throw errors, which while uber-juno itself isn't broken it is effectively broken, since there's no way to stop those errors from being thrown without major edits to the source code. Since until a newer version of atom-ink is published to the Pulsar Package Registry, uber-juno will still throw errors.

But since you've done a fantastic job up until now in getting these maintainers responsive to the issues at hand I'll reword this as recommended but may wait a bit before applying any badging to see if they are able to get newer versions submitted.

Thanks for keeping us up to date!

@goyalyashpal
Copy link

goyalyashpal commented Apr 17, 2023

Unless they do then we still have the issue of the "bad" version being in the backend and a badge will still need to be applied.

yep, i have no problem with the bad badge on uber-juno, sorry if i suggested this as a change (though i think i haven't)... i just suggested these changes in wordings to bring them close to the reality (well, upto as far as i understood upto source level anyway 😅 - as i am pretty sure the packing and deployement part flows slightly over my head)

oh and also, one of the other changes i suggested was in parity with how it's done for other packages for which the update has not been packaged/pushed to pulsar --> the whole manual pulsar -p install ... thing

@confused-Techie
Copy link
Member Author

Alright as it's been two weeks with no new version published by the maintainers of these packages, I'll go ahead and update this here.

Additionally, since the uber-juno package will still install the broken version of the ink package (even if ink has the fixes on the git repo), there's no command we can use to install both from the git repo, as uber-juno will install what's available of ink on the registry.

So with all of that said, both packages are broken for better or worse, although we could include instructions on how to install ink from the git repo only.

Appreciate all the effort @goyalyashpal has put into this one, sorry the maintainers haven't come through to get things updated.

@goyalyashpal
Copy link

goyalyashpal commented Aug 2, 2023

there's no command we can use to install both from the git repo, as uber-juno will install what's available of ink on the registry.

this pretty much did not make sense to me back then. but now, after having a hands on experience with package management as a dev - this becomes perfectly clear.

wow on how well the concepts translate as following

Offtopic

Convergent evolution, node's pnpm boasts similar things about env mgmt & deps resolution as the language agnostic micromamba too lol.

This 2nd option for python uses a different distribution channel, the conda-forge, which allowed me to cope with lack of ready build packages there.

edit:
pnpm add <pkg> uses same repo/registry as npm i.e. MS Github's npmjs.com.
Ref: pnpm.io/cli/add.

So, there doesnt seem to exists any commuity registry.

And well, even if it did then; pulsar would still be limited to repos supported by pac-man it uses, trickling down from ppm@ppr eventually to the npm

so food for my own thoughts: are there diff repos for node deps as well? maybe they can solve not this issue, but at least the #586

what repo does the pnpm use?

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