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

Please revert PR #1522 #1580

Closed
philippeboyd opened this issue Feb 27, 2023 · 4 comments
Closed

Please revert PR #1522 #1580

philippeboyd opened this issue Feb 27, 2023 · 4 comments
Labels
bug Used to mark issues with provider's incorrect behavior

Comments

@philippeboyd
Copy link
Contributor

philippeboyd commented Feb 27, 2023

@sfc-gh-swinkler Why was this PR even merged #1522? It opened up the gates of hell in terms of bugs and new opened issues.

And seriously, emojis in IDs #1541?
image

@philippeboyd philippeboyd added the bug Used to mark issues with provider's incorrect behavior label Feb 27, 2023
@michael-robbins
Copy link
Contributor

They probably accidentally merged it in before April 1st... might be worth holding off until then

@sfc-gh-swinkler
Copy link
Collaborator

sfc-gh-swinkler commented Mar 1, 2023

@philippeboyd there were only a few issues for specific grant (procedures, functions) resources, and these have since been fixed. Note that these issues would have been an problem even without the use of emojis, as I was refactoring the grant resources to fix imports and reads, and this something that i failed to account for in the acceptance testing (there are now tests to cover these cases).

Since the resources are currently working, and still supporting the old ID format, I do not see a need to revert the change. I did not introduce emojis for the sake of emojis, i did it as part of a larger refactoring effort on grant ids, and the use of emojis was incidental, as a way to easily discriminate between old and new ids. I am not going to remove emojis just because you do not like them, however I will refrain from using them in the future since this is evidently a divisive issue.

If you encounter any more bugs please let me know, and I will respond to them, but just saying you do not like something that does not impact the usability of the provider is not constructive feedback.

@PedroMartinSteenstrup
Copy link

Amazing to have to bookmark somewhere a place to copy paste a snowflake character so that I can write my lines of imports. 🚀
Looking forward the thousands I have to do in the next weeks.
Also glad "easily discriminate between old and new ids" could be done at the expense of end-users experience, the ones that actually do use imports 👏

image

@philippeboyd
Copy link
Contributor Author

Amazing to have to bookmark somewhere a place to copy paste a snowflake character so that I can write my lines of imports. 🚀 Looking forward the thousands I have to do in the next weeks. Also glad "easily discriminate between old and new ids" could be done at the expense of end-users experience, the ones that actually do use imports 👏

image

This is precisely the type of problem I was referring to in my #1541 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to mark issues with provider's incorrect behavior
Projects
None yet
Development

No branches or pull requests

4 participants