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

Fix "lib uninstall" when library name contains spaces #443

Merged
merged 1 commit into from Dec 11, 2019

Conversation

howjmay
Copy link
Contributor

@howjmay howjmay commented Oct 15, 2019

Now, the "_" is replaced with a blanket " ", to reslove the
inconsistency problem. However, use the name of the installing
library may be a better solution.

fixes #362

BTW, when we uninstall libraries, there are no messages to notify the user libraries are successfully uninstalled.
However, when we install libraries message like Installed LiquidCrystal I2C@1.1.2 is printed to notify libraries are successfully installed.

@masci
Copy link
Contributor

masci commented Oct 16, 2019

@howjmay howjmay force-pushed the core_search branch 2 times, most recently from cdcd726 to 07c646b Compare October 20, 2019 07:11
@howjmay howjmay changed the title Libraries (un)installation name inconcistency (WIP) Libraries (un)installation name inconcistency Oct 20, 2019
@rsora rsora added the status: in progress Work is in progress on this label Oct 22, 2019
@masci
Copy link
Contributor

masci commented Oct 22, 2019

@howjmay we're almost there, now integration tests are failing here: https://github.com/arduino/arduino-cli/blob/master/test/test_lib.py#L61
You can find the nstructions to run integration tests locally here: https://github.com/arduino/arduino-cli/tree/master/test let me know if you need any guidance, those tests require a Python3 interpreter to run.

@howjmay
Copy link
Contributor Author

howjmay commented Oct 23, 2019

Thanks @masci
It seems I may meet a problem, but I am still digging it.
I will ask for help if I can't solve alone!

@howjmay
Copy link
Contributor Author

howjmay commented Nov 1, 2019

Hi @masci Sorry for answering late. My PC was broken in the last few weeks.
I met a problem that in

func SanitizeName(origName string) string {

Name with space " " would be replace with underline "_", but this would cause a problem if I simply transform an underline back to space. Because the underline may be transformed from other symbol.

I am kind confused that how can I solve this problem. Maybe we need a naming rule for libraries ?

@zmoog
Copy link
Contributor

zmoog commented Nov 8, 2019

Hi @howjmay, unfortunately, we currently don't have a specification for the library names.

The original library name is changed here

saneName := utils.SanitizeName(indexLibrary.Library.Name)

using the SanitizeName() function to be more filesystem friendly and is also used internally as an identifier for the library within the LibrariesManager data structures.

Why don't we limit the use of the sanitized version of the library name inside the librariesmanager package only?

We could change this:

alternatives, have := sc.Libraries[libRef.Name]

into something like this:

    saneName := utils.SanitizeName(libRef.Name)
    alternatives, have := sc.Libraries[saneName]

All the sanitize/normalization stuff would remain inside the librariesmanager package making it easier to change it into something else later on.

@howjmay
Copy link
Contributor Author

howjmay commented Nov 13, 2019

@zmoog Thank you !!
I didn't be aware there is a way like this!!

@howjmay howjmay force-pushed the core_search branch 5 times, most recently from 6d07a91 to e09e367 Compare November 13, 2019 18:22
@howjmay howjmay changed the title (WIP) Libraries (un)installation name inconcistency Libraries (un)installation name inconcistency Nov 13, 2019
@howjmay
Copy link
Contributor Author

howjmay commented Nov 18, 2019

Hi @masci I failed in CI for some unknown issue. Do you know what happen?

@masci
Copy link
Contributor

masci commented Nov 18, 2019

@howjmay my apologies, the CI system is currently disabled due to administrative problems but it'll be back online soon. For the time being I'm afraid you have to rely on running the tests locally, thanks for your patience!

@howjmay
Copy link
Contributor Author

howjmay commented Dec 3, 2019

Hi @masci How is the situation of CI?
Last time I ran the test of this PR, I passed it, even though I met some problems to run tests again.

@masci
Copy link
Contributor

masci commented Dec 3, 2019

@howjmay CI is back, you can add a commit and push (or just force-push last commit) to retrigger

Fix the inconsistent name of libraries when installing
and uninstalling.
@howjmay
Copy link
Contributor Author

howjmay commented Dec 5, 2019

done

@masci masci removed the status: in progress Work is in progress on this label Dec 5, 2019
@masci masci self-requested a review December 5, 2019 16:45
@masci masci added this to the 0.7.0 milestone Dec 11, 2019
Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

LGTM, I wrote an integration test for this feature, I'll push it in a separate PR.
Thanks for your code!

@masci masci changed the title Libraries (un)installation name inconcistency Fix library uninstall when library name contains spaces Dec 11, 2019
@masci masci changed the title Fix library uninstall when library name contains spaces Fix "lib uninstall" when library name contains spaces Dec 11, 2019
@masci masci merged commit 4f3fec6 into arduino:master Dec 11, 2019
@howjmay
Copy link
Contributor Author

howjmay commented Dec 11, 2019

Thank you @masci

masci pushed a commit that referenced this pull request Dec 11, 2019
masci pushed a commit that referenced this pull request Dec 13, 2019
…s installed (#511)

* add test for #443

* return an empty json array when there are no libs
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.

Libraries installation/uninstallation name inconcistency
4 participants